Skip to content

Add library-backed OAuth validation and managed interoperability tests - #80

Merged
kdubb merged 4 commits into
mainfrom
codex/oauth-conformance
Oct 6, 2026
Merged

kdubb merged 4 commits into
mainfrom
codex/oauth-conformance

Conversation

@kdubb

@kdubb kdubb commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

OAuth discovery and token responses previously permitted inconsistent parsing and contextual validation across runtimes. This change adds explicit validation, 123 wire fixtures and 34 matching HTTP classification fixtures, and managed interoperability tests while preserving existing public APIs and the public-client compatibility policy from #59 in sunday-kt.

oauth4webapi 3.8.8 constructs requests and processes token-success responses; Sunday retains explicit wire and contextual policy checks.

Native test infrastructure owns checksum/digest-pinned WireMock replay and Keycloak 26.2.5 live lifecycle. Replay is the default. On macOS CI, live tests select the official Java distribution without Docker discovery; other environments use the container. CI invokes standard test commands with replay for PRs and live on main.

Validation: bun run check passes: lint, typecheck, 773 tests, and build. Managed replay and live Keycloak acquisition/refresh pass for public PKCE and confidential Basic/POST. Includes a regression fix that clears superseded event reconnect timers and closes test-owned event sources during teardown; the existing Husky 4 hook was broken locally, so checks were run directly before committing.

Acceptance follow-up: all five transports now assert exact HTTP error categories during acquisition and refresh; accepted wire cases assert token values and absolute expiry. Recognized discovery endpoint validation is aligned across runtimes. Native harness tests cover startup failure, readiness timeout, process cleanup, corrupt caches, and rejected downloads. Dependency footprint and license measurements are recorded in the evaluation report.

Local replay/full checks and live Keycloak acquisition/refresh pass using both the Java distribution and pinned container. Kotlin additionally prevents OkHttp from automatically replaying single-use token requests on 408 or immediate-retry 503 responses. No live Auth0 tenant was tested; Auth0-shaped fixtures are deterministic only. Current-head hosted checks and review remain the merge gates. This PR does not publish a beta.

Error classification tolerates empty advisory descriptions as a documented interoperability exception while rejecting wrong types. Fractional expiry values are checked before numeric rounding, with matching precision-boundary fixtures across runtimes. JavaScript pins lossless-json 4.3.0 for numeric validation. Failed container cleanup is reported and can be retried. The intentionally deprecated oauth4webapi HTTP option remains guarded by the loopback-only endpoint policy.

Related implementation PRs: Kotlin #61, JavaScript #80, Python #7, Swift #73.

Use oauth4webapi for protocol requests and token processing behind the existing fetch API. Add strict policy checks, aligned fixtures, and managed WireMock/Keycloak tests with macOS CI Java selection. Clear superseded event reconnect timers and cover the regression exposed by the full suite.
kdubb added 2 commits October 5, 2026 14:37
Preserve the newly landed discovery regressions, supply the required authorization endpoint in the refresh fixture, and close all event-source fixtures during teardown.
Align recognized discovery endpoint validation, assert normalized token values and shared acquisition/refresh error categories, and cover provider startup and artifact failures. Record resolved dependency footprints and live backend verification.
@kdubb
kdubb marked this pull request as ready for review October 5, 2026 22:45
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[High risk] Adds OAuth library dependencies and wire protocol handling.

The reviewed changes appear safe to merge; no new blocking issue was found.

What we checked:

  • Failed cleanup can be retried: Provider.close keeps the container name when removal fails and throws after local cleanup. A later call retries removal.

Summary

Adds library-backed OAuth requests and response checks, shared fixtures, and managed replay/live tests.

  • The follow-up checks expiry numbers before conversion and accepts empty advisory error descriptions.
  • Both earlier unnumbered findings are fixed: empty descriptions no longer hide invalid_grant, and failed container removal is reported and can be retried.
  • No new actionable issues were found.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Acquire or refresh] --> B[Resolve and check endpoints]
  B --> C[oauth4webapi builds token request]
  C --> D[Sunday fetch]
  D --> E{HTTP result}
  E -->|Temporary failure| F[Temporary error]
  E -->|Other non-200| G[Read OAuth error code]
  E -->|200| H[Check raw expiry and scope]
  H --> I[oauth4webapi checks token response]
  I --> J[Check requested scopes and absolute expiry]
  J --> K[Return tokens]
Loading

Reviews (2) · Last reviewed commit: "Fix OAuth review findings and align pars..."

Comment thread src/oauth-wire.ts Outdated
Comment thread test/oauth-support/provider.ts Outdated
Preserve refresh error classification for empty advisory descriptions and validate expiry before numeric rounding. Align shared fixtures, repair reviewed test lifecycle behavior, and document protocol compatibility and dependency boundaries.
@sonarqubecloud

sonarqubecloud Bot commented Oct 6, 2026

Copy link
Copy Markdown

@kdubb

kdubb commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Sonar follow-up: the nested authentication ternary and generic length assertion are fixed. The remaining allowInsecureRequests deprecation is intentional in oauth4webapi 3.8.8; its declaration says the annotation is meant to discourage non-TLS use, and there is no replacement option. Sunday validates endpoints before enabling it and permits HTTP only for loopback hosts. Remote HTTP and redirect rejection remain covered by tests. The rationale is recorded beside the option and in the evaluation report.

Current-head validation: bun run check passes (773 tests), the Node ESM smoke check passes, and managed Keycloak Java/container acquisition and refresh pass for public PKCE and confidential Basic/POST clients. Hosted build/test and Sonar quality gate also pass for 2338aaf.

@kdubb
kdubb merged commit 8154d13 into main Oct 6, 2026
3 checks passed
@kdubb
kdubb deleted the codex/oauth-conformance branch October 6, 2026 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant