added validation around code_challenge_method to reject non S256 values by bwang-icf · Pull Request #1599 · CMSgov/bluebutton-web-server

bwang-icf · 2026-05-19T22:24:25Z

JIRA Ticket:
BB2-4805

What Does This PR Do?

Rejects any non S256 code challenge method calls

What Should Reviewers Watch For?

We already have some validation in the form of check_required_params and such within this file. Would it make sense to condense some of these?

Validation

Spin up a local instance and attempt an auth flow. Copy the authorize url and change the S256 value to something else. Observe a failed request and then attempt a regular workflow and see that it passes.

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

Adds any new software dependencies
Modifies any security controls
Adds new transmission or storage of data
Any other changes that could possibly affect security?

Yes, one or more of the above security implications apply. This PR must not be merged without the ISSO or team
security engineer's approval.

Any Migrations?

Yes, there are migrations
- The migrations should be run PRIOR to the code being deployed
- The migrations should be run AFTER the code is deployed
- There is a more complicated migration plan (downtime,
  etc)
No migrations

jimmyfagan

Looks good, just some comments to hopefully make this a bit cleaner hopefully.

jimmyfagan · 2026-05-19T23:40:13Z

+        # Validating before medicare login step that code_challenge_method is correct as per BB2-4805
+        # We could consider adding other checks at this step as well. Open to the team's suggestions


I'd prefer keeping these kinds of comments out of the code, feel free to start convos in the PR, but I don't think these comments are useful to potentially stay in the code for a while after merging.

This can be removed for sure.

jimmyfagan · 2026-05-19T23:54:19Z

+        validation_error = self._validate_code_challenge_method(code_challenge_method)
+        if validation_error:
+            return validation_error


Feels like this is probably better coming after the _check_for_required_parameters call. Check that the required params exist before checking for validity of values.

jimmyfagan · 2026-05-20T00:11:00Z

+    def _validate_code_challenge_method(self, code_challenge_method):
+        """Validate code_challenge_method is S256 if provided.
+
+        Returns None if valid, JsonResponse error if invalid.
+        """
+        if code_challenge_method and code_challenge_method != 'S256':
+            return JsonResponse(
+                {
+                    'status_code': HTTPStatus.BAD_REQUEST,
+                    'error': 'invalid_request',
+                    'error_description': f'code_challenge_method must be S256, got: {code_challenge_method}',
+                },
+                status=HTTPStatus.BAD_REQUEST,
+            )
+        return None


Wondering if we can move this to _check_for_required_params similar to what we do when we confirm the existence of state and then validate it in an elif. And then maybe we can add a call to _check_for_required_params to dispatch so we do these checks pre-login as you suggested. How does that sound? If we do that, it might invalidate one of my other comments.

Yeah, I think it can be there. That answers my other comment from before about where else that check might live. I'll change it so it's part of the required_param check

jimmyfagan · 2026-05-20T00:11:55Z

+            return validation_error
        kwargs['code_challenge'] = request.POST.get('code_challenge')
-        kwargs['code_challenge_method'] = request.POST.get('code_challenge_method')
+        kwargs['code_challenge_method'] = code_challenge_method


Was this necessary? Kind of unclear to me why any of the kwargs need to be manually set like this to be honest.

I think otherwise the PKCE params don't show up in the super().post. It needs to be set within kwargs.

Why is it needed in super().post? Seems like it was working fine without this before

This could be reverted back, but I was mostly just following what was currently within the code. I think currently the code in master does set the kwargs here, but I can definitely try to run this without setting kwargs and see what happens and remove these if they're really not needed.

Looks like these indeed are not needed anymore, so we're good removing this. Some chance for related refactors in form_valid to uncomment the lines 398-399, and then removing the ifs for those values beneath that. Seems like these are all kind of remnants of when PKCE was first added, so some cleanup in this area would be nice while we're at it.

Agent-Logs-Url: https://github.com/CMSgov/bluebutton-web-server/sessions/ec2649c7-d427-4f59-ad18-098613b75e4e Co-authored-by: bwang-icf <178809349+bwang-icf@users.noreply.github.com>

jimmyfagan

Looks good, and seems to work as expected. Left a few more comments, should be good for a final approval after those last couple notes.

jimmyfagan · 2026-05-21T22:07:15Z

+        code_challenge_method = request.POST.get('code_challenge_method')
+        validation_error = self._validate_code_challenge_method(code_challenge_method)
+        if validation_error:
+            return validation_error


Could we replace _validate_code_challenge_method with _check_for_required_params here too? If so, then let's get _validate_code_challenge_method removed altogether.

jimmyfagan · 2026-05-21T22:16:44Z

+            return validation_error
        kwargs['code_challenge'] = request.POST.get('code_challenge')
-        kwargs['code_challenge_method'] = request.POST.get('code_challenge_method')
+        kwargs['code_challenge_method'] = code_challenge_method


Looks like these indeed are not needed anymore, so we're good removing this. Some chance for related refactors in form_valid to uncomment the lines 398-399, and then removing the ifs for those values beneath that. Seems like these are all kind of remnants of when PKCE was first added, so some cleanup in this area would be nice while we're at it.

…s on auth

added validation around code_challenge_method to reject non S256 values

9a024ed

jimmyfagan reviewed May 20, 2026

View reviewed changes

bwang-icf and others added 2 commits May 20, 2026 15:17

Merge branch 'master' into brandon/BB2-4805-invalid-code-challenge

dcb968c

intermediate commit

ce47501

Copilot started work on behalf of bwang-icf May 21, 2026 21:00 View session

Copilot AI and others added 2 commits May 21, 2026 21:06

Fix authorize POST param handling in code_challenge_method validation

058e653

Agent-Logs-Url: https://github.com/CMSgov/bluebutton-web-server/sessions/ec2649c7-d427-4f59-ad18-098613b75e4e Co-authored-by: bwang-icf <178809349+bwang-icf@users.noreply.github.com>

Remove stale commented lines in authorization GET handler

dabe0f9

Agent-Logs-Url: https://github.com/CMSgov/bluebutton-web-server/sessions/ec2649c7-d427-4f59-ad18-098613b75e4e Co-authored-by: bwang-icf <178809349+bwang-icf@users.noreply.github.com>

Copilot finished work on behalf of bwang-icf May 21, 2026 21:09

jimmyfagan reviewed May 21, 2026

View reviewed changes

bwang-icf and others added 5 commits May 22, 2026 09:03

cleanup of previous pkce setup

31a2868

Merge branch 'master' into brandon/BB2-4805-invalid-code-challenge

a771246

removing unnecessary error handling

020d8e4

Merge branch 'master' into brandon/BB2-4805-invalid-code-challenge

e7377c4

Merge branch 'master' into brandon/BB2-4805-invalid-code-challenge

a8178a4

bwang-icf marked this pull request as draft May 28, 2026 14:15

adding updated tests with pkce params as part of enforcing pkce param…

095d7d0

…s on auth

		# Validating before medicare login step that code_challenge_method is correct as per BB2-4805
		# We could consider adding other checks at this step as well. Open to the team's suggestions

Conversation

bwang-icf commented May 19, 2026

What Does This PR Do?

What Should Reviewers Watch For?

Validation

What Security Implications Does This PR Have?

Any Migrations?

Uh oh!

jimmyfagan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmyfagan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants