Exclude 2FA from upload#show routes and 404s
What does this MR do?
As seen in #247461 (closed), nonexistent routes should not redirect to the 2FA page, as there can be missing resources, which would cause the 2FA secret to be regenerated
For UploadsController#show routes, any resource trying to be loaded while on the 2FA page (avatar, custom header etc) would redirect to the 2FA page, also regenerating the token after the QR/key was already rendered in the view.
This skips the 2FA check for UploadsController#show
routes and ApplicationController#route_not_found
ones. The latter is correct, but the UploadsController#show
may not be the best idea from a security p.o.v. - it would allow users with enforced 2FA to still view uploads if they had a direct link.
WDYT @dblessing @mksionek?
Screenshots
Does this MR meet the acceptance criteria?
Conformity
-
Changelog entry -
Documentation (if required) -
Code review guidelines -
Merge request performance guidelines -
Style guides -
Database guides -
Separation of EE specific content
Availability and Testing
-
Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process. -
Tested in all supported browsers -
Informed Infrastructure department of a default or new setting change, if applicable per definition of done
Security
If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Merge request reports
Activity
Another possible solution would be to have the 2FA secret code depend on the session instead of re-generating every time the page reloads, instead of this MR (users would still see images not loading, as before - seems like it's been happening for a long time - but now it won't affect their ability to set a 2FA).
added backend label
2 Warnings ⚠ This merge request does not refer to an existing milestone. ⚠ You’ve made some app changes, but didn’t add any tests.
That’s OK as long as you’re refactoring existing code,
but please consider adding any of the ~”tooling”, ~”tooling::pipelines”, ~”tooling::workflow”, ~”documentation”, ~”QA” labels.Reviewer roulette
Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.
To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited, or the chosen person is unavailable.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, mention them as you normally would! Danger does not automatically notify them for you.
Category Reviewer Maintainer backend Andy Soiron ( @Andysoiron
) (UTC+2)Dmitriy 'DZ' Zaporozhets ( @dzaporozhets
) (UTC+3)If needed, you can retry the
danger-review
job that generated this comment.Generated by
🚫 DangerEdited by 🤖 GitLab Bot 🤖I've also tried adding tests around route_not_found in the ApplicationController specs but the action won't be
route_not_found
for the before_action skip, rather thanindex
, not sure if there's a better way of doing it or this should be an e2e test instead.🤔 i.e. this doesn't work as expected:modified spec/controllers/application_controller_spec.rb @@ -195,6 +196,31 @@ RSpec.describe ApplicationController do expect(response).to redirect_to new_user_session_path end + + describe 'when 2fa is required' do + before do + sign_in(user) + end + + it 'does not redirect when 2fa is required on application level' do + stub_application_setting require_two_factor_authentication: true + + get :index + + expect(response).not_to redirect_to profile_two_factor_auth_path + end [..]
added 1 commit
- 5456e52c - Exclude 2FA from upload#show routes and 404s
added 1 commit
- 6e0a0b2e - Exclude 2FA from upload#show routes and 404s
assigned to @dblessing and unassigned @cat
mentioned in issue #247461 (closed)
@rchan-gitlab This was technically caused by the fix for #27686 (closed).
Do you have any thoughts on the best way forward? I'm guessing we don't want to revert the MR.
Do you have concerns with the approach here? The one concern I have is the behavior when 2FA grace period expires - in that case technically uploads and static assets will still be accessible even if the account is 'locked out' until it enables 2FA.
We may need to think in terms of a 2-stage fix - one to get out in a patch release this week and one for a longer term fix. It's affecting quite a few customers.
- Resolved by James Fargher
This approach sounds like a good temp fix.
The one concern I have is the behavior when 2FA grace period expires - in that case technically uploads and static assets will still be accessible even if the account is 'locked out' until it enables 2FA.
I'm not sure what does it mean. Do you mean even the user would be able to bypass the 2FA requirement to be able to view the protected resources with this fix?
I'm thinking about to take on another direction, what about we change the route
/profile/two_factor_auth
to accept POST request only as a second stage fix. Because now it looks like the user is CSRFing themselves without knowing it, causing the invalid PIN errors, usually to fix CSRF problems, enforcing CSRF protection is the way to go, in this case, change GET to POST sounds like an easy fix to me.Edited by Ron Chan
added severity2 label
mentioned in merge request !43178 (closed)
I'm OK with this fix as-is, since it's a short term fix. Let's get it out there and then we'll follow up with !43178 (closed).
@proglottis Do you mind doing a backend maintainer review, please?
For some added context, this is a quick fix for an ~S2 regression. It's OK'd by AppSec. Long term we want to address it in a different way, and will aim to do so in the current regular milestone (such as by making the secret live for the duration of a given session).
enabled an automatic merge when the pipeline for 68c07368 succeeds
- Resolved by Drew Blessing
Looks like the failure is unrelated: #255176 (closed)