You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Ignore UpdateUserLocaleAction to force a result - #54250
The issue was that the UpdateUserLocaleAction did not set any result. The PR adds the ignore and also change the evaluation to default to failure instead of returning null (the most secure option I think). I have reviewed the other actions and I see all of them manage the result, but for example the UpdateEmail was doing the same in some scenarios. Fixed too. Let's see if copilot finds more actions that are not adding the result status in some paths (but now the evaluation will fail instead of silently returning early).
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The security-sensitive dispatcher change affects all required-action providers and warrants human validation, with direct unset-status regression coverage still missing.
No open issues 🎉
1 previously missed issue
In code that hasn't changed since last review
Add regression test for unset challenge status rejection
The added locale regression test takes the IGNORE branch, so it would still pass if this unset-status fallback returned null again. Please add a regression test with a required-action provider whose requiredActionChallenge leaves the status unset, asserting that authentication is rejected and no authorization code is issued.
The reason will be displayed to describe this comment to others. Learn more.
@rmartinc Thanks for the PR! I've just added one comment.
Btw. the signature of methods for the required actions might be improved, as we'd like to always mark the execution somehow. It means that at this moment, we need to add the missing context.XyAction()(ignore, challenge,...). It'd be nice to have some better method signatures that could always return some kind RequiredActionResult with the status + possible body. But it might be for the future. WDYT?
The reason will be displayed to describe this comment to others. Learn more.
Should we really remove it here? What if the update email functionality is enabled afterwards? IMHO, we should just ignore it, and still have it recorded on the user, right? This is kind of behavioral change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #54244
The issue was that the UpdateUserLocaleAction did not set any result. The PR adds the ignore and also change the evaluation to default to failure instead of returning null (the most secure option I think). I have reviewed the other actions and I see all of them manage the result, but for example the UpdateEmail was doing the same in some scenarios. Fixed too. Let's see if copilot finds more actions that are not adding the result status in some paths (but now the evaluation will fail instead of silently returning early).