Skip to content

Implement AuthenticationMiddleware (#117)#171

Open
leogdion wants to merge 2 commits into
claude-promptfrom
117-authentication-middleware
Open

Implement AuthenticationMiddleware (#117)#171
leogdion wants to merge 2 commits into
claude-promptfrom
117-authentication-middleware

Conversation

@leogdion

Copy link
Copy Markdown
Member

Closes #117

Summary

Adds Sources/AiSTKit/AuthenticationMiddleware.swift — a public OpenAPI ClientMiddleware that injects the Anthropic authentication headers into every outgoing request:

  • x-api-key with the stored API key
  • anthropic-version: 2023-06-01

The request is then forwarded to the next handler via next(request, body, baseURL). The middleware will be wired into the generated ClaudeKit client by the ClaudeKit wrapper in #116.

Implementation notes

  • Implemented against the actual swift-openapi-runtime 1.x ClientMiddleware signature (includes body: HTTPBody? and returns (HTTPResponse, HTTPBody?)) — the simplified snippet in Docs/skit-analyze-plan.md §I2 predates the current protocol shape.
  • Uses public import for OpenAPIRuntime, HTTPTypes, and Foundation.URL since the package builds with InternalImportsByDefault and these types appear in the public conformance.
  • Avoids force-unwrapping HTTPField.Name (SwiftLint force_unwrapping rule) via if let on the always-valid header names.

Acceptance criteria

  • AuthenticationMiddleware implements ClientMiddleware protocol
  • intercept() adds x-api-key header with API key
  • intercept() adds anthropic-version header
  • Middleware correctly forwards request to next handler
  • Code follows one-type-per-file organization

Verification

  • swift build — clean
  • swift test — 489 tests pass
  • ./Scripts/lint.sh — SwiftFormat, SwiftLint, header check, and test build pass (Periphery's "index store path does not exist" error reproduces on a clean checkout, pre-existing local environment issue; Periphery is skipped in CI)

🤖 Generated with Claude Code

Adds a public ClientMiddleware that injects the x-api-key and
anthropic-version headers into every outgoing ClaudeKit request.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fb842e7d-af83-4da0-ae62-bd84163e1619

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 117-authentication-middleware

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review: Implement AuthenticationMiddleware (#117)

Overview

Adds AuthenticationMiddleware, an OpenAPI ClientMiddleware that injects x-api-key and anthropic-version headers into every outgoing request. Clean, focused implementation — one type per file, correct protocol conformance, good use of public import scoping.


Issues

Silent header omission is a latent security bug

if let name = HTTPField.Name("x-api-key") {
    request.headerFields[name] = apiKey
}

HTTPField.Name.init is failable but will never return nil for these hardcoded, valid ASCII strings. The if let guard silently swallows a failure — if it ever fires, the request goes out without authentication headers, which is worse than crashing. The correct pattern is either:

  • guard let + throw a descriptive error, or
  • a force unwrap with a comment explaining these literals are always valid:
// Safety: these literals satisfy HTTPField.Name's ASCII constraints.
let apiKeyName = HTTPField.Name("x-api-key")!
let versionName = HTTPField.Name("anthropic-version")!

anthropicVersion not configurable

The API version is a private static constant. Anthropic does introduce breaking API version bumps. Consider making it injectable with a default value so callers can opt in to newer versions without modifying this type:

public init(apiKey: String, anthropicVersion: String = "2023-06-01") { ... }

Test Coverage

No tests are included. Suggested cases:

  • Verify both headers are present in the forwarded request.
  • Confirm the original body and base URL pass through unchanged.
  • Confirm the response tuple is returned unmodified.

A lightweight mock for the next closure that captures the mutated request makes these trivial to write.


Minor

  • File header says SyntaxKit in the module comment but this file lives in AiSTKit — cosmetic mismatch.

Overall this is a solid, minimal implementation. The silent-drop concern is the main thing worth addressing before merge.

🤖 Generated with Claude Code

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — AuthenticationMiddleware (#117)

Overview

This PR adds a single-file OpenAPI ClientMiddleware that injects x-api-key and anthropic-version headers into every outgoing Anthropic API request. The implementation is clean and narrowly scoped.


What's Good

  • Correct public import usage under InternalImportsByDefault — avoiding these would cause build failures on the protocol conformance.
  • @preconcurrency guard for Linux Foundation.URL is the right platform-specific workaround.
  • Struct is simple, all-let state — implicit Sendable conformance is safe and correct.
  • The file header, license block, and doc-comment style all match the project's conventions.

Issues

1. No unit tests (required by CLAUDE.md)

CLAUDE.md step 3 in Adding New Syntax Elements requires unit tests in Tests/SyntaxKitTests/Unit/. No test file is included here, and the PR description only confirms existing tests still pass. At minimum, a test should verify:

  • That x-api-key is set to the provided key.
  • That anthropic-version is set to the expected constant.
  • That next is called with the modified request and the original body/baseURL.

A simple mock next closure is enough — no network required.

2. Silent auth bypass on header-name construction failure

if let name = HTTPField.Name("x-api-key") {
    request.headerFields[name] = apiKey
}

HTTPField.Name.init(_:) returns nil for invalid field names. For known-valid compile-time constants like "x-api-key" and "anthropic-version" this will never be nil, but if it somehow were, the request would be forwarded without authentication headers — a silent security regression rather than a loud failure.

Options:

  • Use the typed HTTPField.Name constants if HTTPTypes exposes them (e.g. HTTPField.Name.authorization-style), and fall back to a preconditionFailure / assertionFailure with a clear message for the custom names.
  • Or add a comment explicitly acknowledging the invariant so a future reader doesn't "fix" it by removing the guard.

3. Anthropic version is not configurable

anthropicVersion is a private static let with no way for callers to override it. When Anthropic releases a new API version, every consumer must update and redeploy the library. Consider accepting it as an optional init parameter with a default:

public init(apiKey: String, anthropicVersion: String = "2023-06-01") {
    self.apiKey = apiKey
    self.anthropicVersion = anthropicVersion
}

This keeps the current behaviour as the default while allowing integrators to pin to a newer version without waiting on a library release.

4. Minor: file header says "SyntaxKit"

//  AuthenticationMiddleware.swift
//  SyntaxKit        ← should be AiSTKit

The // <ModuleName> line in the header should match the target (AiSTKit), not the top-level package.


Summary

Severity Issue
Required Add unit tests per CLAUDE.md convention
Medium Silent auth bypass if HTTPField.Name init returns nil — at minimum document the invariant
Low Make anthropicVersion configurable via init
Nit Fix module name in file header

The core logic is correct and the implementation matches the protocol shape accurately. Addressing the missing tests and the silent-failure path are the blockers before merge.

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.49%. Comparing base (07f505b) to head (bd70fdb).
⚠️ Report is 1 commits behind head on claude-prompt.

❗ There is a different number of reports uploaded between BASE (07f505b) and HEAD (bd70fdb). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (07f505b) HEAD (bd70fdb)
ubuntu 6 1
swift-6.1 2 0
swift-6.3 2 1
swift-6.2 2 0
spm 2 1
Additional details and impacted files
@@                Coverage Diff                 @@
##           claude-prompt     #171       +/-   ##
==================================================
- Coverage          80.16%   65.49%   -14.68%     
==================================================
  Files                153      185       +32     
  Lines               5209     6378     +1169     
==================================================
+ Hits                4176     4177        +1     
- Misses              1033     2201     +1168     
Flag Coverage Δ
spm 65.43% <100.00%> (-14.52%) ⬇️
swift-6.1 ?
swift-6.2 ?
swift-6.3 65.47% <100.00%> (-14.61%) ⬇️
ubuntu 65.47% <100.00%> (-14.69%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — PR 171: Implement AuthenticationMiddleware

Straightforward, correct implementation. One concern worth addressing.

Potential silent auth failure

The if let guards around HTTPField.Name initialisation will silently drop the authentication headers if the name string is ever considered invalid:

if let name = HTTPField.Name("x-api-key") {
    request.headerFields[name] = apiKey
}
// if name is nil, the request goes out WITHOUT the x-api-key header

The PR description explains this was chosen to satisfy SwiftLint's force_unwrapping rule, which is correct. However, a silent no-op on auth failure is a hard-to-debug failure mode — the request will succeed (from the network's perspective) but get a 401 from the API, with no indication at the call site that the header was never set.

Recommendation: Replace the if let with a guard ... else { preconditionFailure(...) } for these compile-time-constant strings. A preconditionFailure is stripped in release builds but will surface the bug immediately in any debug/test run. Alternatively, declare the HTTPField.Name values as private static let constants on the type so the failure is caught at type-load time. This requires a // swiftlint:disable:next force_unwrapping annotation plus a brief comment explaining why the force-unwrap is safe — a future typo in the header name will then fail fast at startup rather than silently sending unauthenticated requests.


Minor observations

  • Hardcoded anthropic-version: 2023-06-01: fine as a private static let constant; consider whether it belongs in a shared constants file alongside the model identifier in AnalyzerConfiguration.
  • public import usage for HTTPTypes, OpenAPIRuntime, and Foundation.URL is correct under Swift 6's InternalImportsByDefault since these types appear in the public API surface. ✓
  • @preconcurrency on Linux only matches the upstream behaviour of Foundation.URL's Sendable conformance availability. ✓
  • One-type-per-file, explicit access control — all consistent with project conventions. ✓

@claude

claude Bot commented Jun 14, 2026

Copy link
Copy Markdown

Code Review — AuthenticationMiddleware

Overview

Adds AuthenticationMiddleware, a ClientMiddleware that injects the two required Anthropic authentication headers (x-api-key and anthropic-version) into every outgoing OpenAPI request. The implementation is clean and well-documented overall, but there are a few issues that should be addressed before merge.


Issues

Missing direct dependencies in Package.swift

The new file uses public import OpenAPIRuntime and public import HTTPTypes inside the AiSTKit target, but Package.swift only lists ClaudeKit and SyntaxParser as direct dependencies of that target. OpenAPIRuntime and HTTPTypes reach AiSTKit only transitively through ClaudeKit.

Using public import of transitively-available modules without declaring them as direct dependencies is fragile — if ClaudeKit's own dependency tree changes, AiSTKit silently breaks. Both products should be added explicitly:

.target(
  name: "AiSTKit",
  dependencies: [
    "ClaudeKit",
    "SyntaxParser",
    .product(name: "OpenAPIRuntime", package: "swift-openapi-runtime"),
    .product(name: "HTTPTypes",      package: "swift-http-types")
  ],
  swiftSettings: swiftSettings
),

Silent authentication failure via if let

if let name = HTTPField.Name("x-api-key") {
  request.headerFields[name] = apiKey
}
if let name = HTTPField.Name("anthropic-version") {
  request.headerFields[name] = Self.anthropicVersion
}

If either HTTPField.Name initializer returns nil (which it shouldn't for these well-formed strings, but the code treats it as possible), the request proceeds to the server without the corresponding auth header. The caller gets no error — just an eventual 401 that can be hard to trace.

For compile-time-known, always-valid header names the canonical fix is a static let initialized once at type load, making any misconfiguration a crash on startup rather than a silent runtime failure:

// Safe: both strings are valid RFC-7230 header-name tokens.
private static let apiKeyFieldName  = HTTPField.Name("x-api-key")!
private static let versionFieldName = HTTPField.Name("anthropic-version")!

If the SwiftLint force_unwrapping rule fires, add a targeted disable comment with a short justification, or configure an exclusion in .swiftlint.yml scoped to this file.

No unit tests

CLAUDE.md requires tests for every new component (Tests/SyntaxKitTests/Unit/), yet there are no tests in this PR. At a minimum, a test should verify that intercept adds both headers with the correct values, and that the body and response from the next closure are passed through unchanged.


Minor Notes

  • File header says SyntaxKit in the module name field; the actual module is AiSTKit.
  • anthropicVersion is a private static constant — fine. If the Anthropic API ever offers multiple stable versions, consider making this an init parameter with a default; but this is out of scope for now.
  • operationID is unused in the implementation, which is expected — just confirming this is intentional.
  • The @preconcurrency public import struct Foundation.URL guard for Linux is correct and matches the pattern used elsewhere in the package.

Summary

The middleware logic itself is correct for the happy path. The three items to address before merge:

  1. Declare OpenAPIRuntime and HTTPTypes as direct dependencies of AiSTKit in Package.swift.
  2. Replace the if let header-name pattern with static let constants (fail-fast rather than silent skip).
  3. Add unit tests per CLAUDE.md conventions.

Adds an AiSTKitTests target with tests that drive intercept(...) via a
stub next handler, verifying x-api-key and anthropic-version injection,
preservation of existing headers, and request forwarding. Also documents
the throwing behavior of intercept.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jun 14, 2026

Copy link
Copy Markdown

Code Review — PR #171: Implement AuthenticationMiddleware

Overview

This PR adds AuthenticationMiddleware, a ClientMiddleware conformance that injects x-api-key and anthropic-version headers into every outgoing OpenAPI request, plus a matching test target and test suite. The scope is tight and focused.


Code Quality & Style

Positives:

  • public import on HTTPTypes, OpenAPIRuntime, and Foundation.URL is correct given the InternalImportsByDefault build setting — these types appear in the public method signatures.
  • anthropicVersion as a private static let is the right place for a constant that belongs to the type.
  • #if os(Linux) @preconcurrency guard is the right Swift 6 pattern for Foundation.URL on Linux.

Concerns:

  1. Silent header injection failureHTTPField.Name("x-api-key") and HTTPField.Name("anthropic-version") are failable initialisers. With these well-formed names they will never actually fail, but wrapping them in if let means a failure silently drops the auth headers entirely, and the unauthenticated request still proceeds to next. This is the worst possible failure mode for an auth layer — the call succeeds with a 401 instead of a loud crash. Since SwiftLint's force_unwrapping rule is the only thing blocking !, prefer a single static constant that force-unwraps once at type load time (a well-established SwiftLint workaround):

    // swiftlint:disable:next force_unwrapping
    private static let apiKeyName = HTTPField.Name("x-api-key")!
    // swiftlint:disable:next force_unwrapping
    private static let versionName = HTTPField.Name("anthropic-version")!

    Or, if the rule can't be suppressed, at minimum replace the silent if let with preconditionFailure in the else branch so the mistake is caught in any test run.

  2. No API key validationinit(apiKey:) accepts an empty string without complaint. An empty key will produce a request with a blank x-api-key header. A one-line guard catches the obvious mis-call early:

    public init(apiKey: String) {
      precondition(!apiKey.isEmpty, "apiKey must not be empty")
      self.apiKey = apiKey
    }
  3. anthropicVersion is non-configurable — Fine for a v1 implementation, but worth a // TODO: comment noting that this will need updating (or parameterising) when Anthropic bumps the required version.


Test Coverage

Positives:

  • Four targeted tests cover the primary behaviours (header injection, header preservation, request forwarding).
  • The Box<Value> helper with NSLock is a practical, correct solution for capturing values from @Sendable closures.
  • Using #require for infallible initialisations keeps test failures meaningful.

Missing cases:

Scenario Why it matters
Empty apiKey Verifies the guard suggested above, or documents that the current behaviour is intentional
Pre-existing x-api-key header Confirms the middleware replaces (not appends) a stale auth header sent by caller code
operationID variation Low-priority, but confirms the middleware is operation-agnostic

Performance

No concerns — header injection is O(1) and allocation-free beyond the one var request copy.


Security

  • API key is stored as a plain String rather than a SecureBytes / Data type. This is idiomatic Swift and consistent with how OpenAPI middleware conventionally works, but worth flagging if the team later adopts secure memory for credentials.
  • No logging of the API key value anywhere in the implementation — good.

Package.swift

The AiSTKitTests target is correctly wired with swiftSettings. No issues.


Summary

The implementation is clean and the happy-path coverage is solid. The two actionable items before merge are:

  1. Replace the silent if let on HTTPField.Name with a static constant + force_unwrapping suppression comment (or preconditionFailure) so a header-name regression is loud.
  2. Add a test (and optionally a precondition) covering an empty API key.

Everything else is optional polish.

@claude

claude Bot commented Jun 14, 2026

Copy link
Copy Markdown

Code Review Update — AuthenticationMiddleware

This PR has been updated since prior reviews to add tests. The test suite is solid: four cases covering header injection, preservation of existing headers, and request forwarding. Good use of the Box helper for @sendable capture.

Still Outstanding

Silent auth bypass (blocker)

The if let pattern for HTTPField.Name has been flagged in every prior review and remains unchanged. If the init somehow returns nil, the request proceeds to next with no x-api-key header — a silent 401 rather than a crash. This is the worst failure mode for an auth layer.

Canonical fix — static constants, fail-fast at type load (with a swiftlint:disable:next force_unwrapping comment), or a preconditionFailure in the else branch so any regression fails loudly in test runs.

Missing test: pre-existing x-api-key header

The tests confirm headers are added but don't confirm the middleware replaces a stale key already in the request.

Package.swift: OpenAPIRuntime and HTTPTypes not declared as direct AiSTKit dependencies

These arrive via ClaudeKit transitively. If ClaudeKit's dependency tree changes, AiSTKit silently breaks. Both should be listed as explicit products in the AiSTKit target's dependencies array.

Nit: File header still reads SyntaxKit — should be AiSTKit.

Tests added since last review are a solid improvement. The silent-bypass issue is the one functional concern that needs resolving before merge.

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