[yedmalaaravind]

Part of #1223.

Adds 16 HTTP regression tests for device-flow PKCE enforcement, covering the existing security fix GHSA-h93p-35jm-rj83. These tests exercise the public OAuth2 endpoints so the coverage remains useful as OpenAM migrates away from Restlet.
What this covers

• Rejection of unsupported or explicitly empty challenge methods.
• Successful exchanges using S256, explicit plain, and the default plain method.
• Rejection of missing or incorrect verifiers before and after device approval.
• Successful redemption of the same code after a rejected attempt, confirming that an invalid verifier does not consume the legitimate client’s code.
• Compatibility with device flows that do not supply a PKCE challenge.
• Verification that issued access tokens identify the expected user through userinfo.
  The shared fixtures provision a dedicated public client and configure the provider settings required by the device-flow tests while preserving unrelated configuration.

This contribution covers the device-PKCE portion of #1223; it does not close the entire issue.

[maximthomas]

Hi @yedmalaaravind,
Thank you for the contribution!
But the integration tests are failing and the Dockerfile changes are outside the scope of the task. Is there a motivation behind forcing IPv4 for the docker image?

[vharseko]

Thank you for adding this coverage. I found two issues:

1. [P1] Carry the resource-owner session into each test request
   device-pkce-test.spec.mjs:53–57

   sessionToken is obtained with the beforeAll request context, but each test receives a separate Playwright request context. The approval request sends the token as an iPlanetDirectoryPro header, while the device endpoint resolves the resource-owner session from the session cookie or URL. Consequently, the CSRF check cannot match the submitted value to a session.

   This explains the integration failure already reported above: in the Docker CI run, all 12 tests that approve a code received 400 bad_request at this assertion; only the four issuance-rejection tests passed. Please authenticate in the test’s request context or explicitly carry the session cookie into it, then rerun the suite.

2. [P2] Avoid overwriting concurrent provider configuration changes
   oauth2-fixtures.mjs:225–241

   This full-object PUT is based on an earlier GET and has no concurrency guard. For example, if the provider initially lacks profile, this helper and ensureProviderConfig can read the same configuration; the latter can add the UMA scopes and saved-consent attribute, after which this PUT writes the older object back and removes those additions. The subsequent reread checks only this helper’s three settings, so it can report success while sibling tests have lost their configuration. A sibling PUT after the reread can conversely undo this helper’s changes.

   Please coordinate provider updates using a concurrency-safe mechanism, or isolate these settings in a separate realm. Merely rereading after the PUT does not prevent a later stale write.

After the tests pass, please also include the requested red/green evidence from #1223: run the relevant tests against a WAR with the DeviceCodeResource fix reverted and show which assertions fail.

[yedmalaaravind]

Hi @yedmalaaravind,
Thank you for the contribution!
But the integration tests are failing and the Dockerfile changes are outside the scope of the task. Is there a motivation behind forcing IPv4 for the docker image?

[yedmalaaravind]

Hi @yedmalaaravind,
Thank you for the contribution!
But the integration tests are failing and the Dockerfile changes are outside the scope of the task. Is there a motivation behind forcing IPv4 for the docker image?

HI @maximthomas Thank you for reviewing this. I added the IPv4 setting after the CI logs showed APT package downloads failing with “Network is unreachable” for IPv6 addresses. The image build then failed, leading to the subsequent “manifest unknown” error.

My intention was to unblock validation of the PKCE tests, but I agree that the Dockerfile changes are outside the scope of this task. I’ll remove those changes from this PR

[yedmalaaravind]

Thank you for adding this coverage. I found two issues:

1. [P1] Carry the resource-owner session into each test request
   device-pkce-test.spec.mjs:53–57
sessionToken is obtained with the beforeAll request context, but each test receives a separate Playwright request context. The approval request sends the token as an iPlanetDirectoryPro header, while the device endpoint resolves the resource-owner session from the session cookie or URL. Consequently, the CSRF check cannot match the submitted value to a session.
   This explains the integration failure already reported above: in the Docker CI run, all 12 tests that approve a code received 400 bad_request at this assertion; only the four issuance-rejection tests passed. Please authenticate in the test’s request context or explicitly carry the session cookie into it, then rerun the suite.
2. [P2] Avoid overwriting concurrent provider configuration changes
   oauth2-fixtures.mjs:225–241
   This full-object PUT is based on an earlier GET and has no concurrency guard. For example, if the provider initially lacks profile, this helper and ensureProviderConfig can read the same configuration; the latter can add the UMA scopes and saved-consent attribute, after which this PUT writes the older object back and removes those additions. The subsequent reread checks only this helper’s three settings, so it can report success while sibling tests have lost their configuration. A sibling PUT after the reread can conversely undo this helper’s changes.
   Please coordinate provider updates using a concurrency-safe mechanism, or isolate these settings in a separate realm. Merely rereading after the PUT does not prevent a later stale write.

After the tests pass, please also include the requested red/green evidence from #1223: run the relevant tests against a WAR with the DeviceCodeResource fix reverted and show which assertions fail.

Thank you for the review. I have removed the Dockerfile changes and addressed both test-fixture issues:

• Device approval now carries the resource-owner session cookie alongside the CSRF token.
• The suite configures an isolated realm instead of updating the shared provider configuration. It also creates a unique resource owner and cleans up the test resources.

I completed local WAR-based validation against running OpenAM servers:

• Green: all 16 tests passed.
• Red: rebuilt the WAR with only the DeviceCodeResource challenge-method validation block removed. All 16 tests ran: the four invalid-method rejection cases failed, and the remaining 12 passed. The failures received HTTP 200 instead of the expected HTTP 400.
• Green again: restarted the preserved green server and reran the same suite; all 16 tests passed.

The screenshots are attached. This demonstrates regression detection for the issuance-method validation.

[maximthomas]

LGTM