Implement AuthenticationMiddleware (#117)#171
Conversation
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Code Review: Implement AuthenticationMiddleware (#117)OverviewAdds IssuesSilent header omission is a latent security bugif let name = HTTPField.Name("x-api-key") {
request.headerFields[name] = apiKey
}
// Safety: these literals satisfy HTTPField.Name's ASCII constraints.
let apiKeyName = HTTPField.Name("x-api-key")!
let versionName = HTTPField.Name("anthropic-version")!
|
Code Review —
|
| 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 Report✅ All modified and coverable lines are covered by tests.
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code Review — PR 171: Implement AuthenticationMiddleware Straightforward, correct implementation. One concern worth addressing. Potential silent auth failure The 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 headerThe PR description explains this was chosen to satisfy SwiftLint's Recommendation: Replace the Minor observations
|
Code Review — AuthenticationMiddlewareOverviewAdds IssuesMissing direct dependencies in Package.swiftThe new file uses Using .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
|
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>
Code Review — PR #171: Implement AuthenticationMiddlewareOverviewThis PR adds Code Quality & StylePositives:
Concerns:
Test CoveragePositives:
Missing cases:
PerformanceNo concerns — header injection is O(1) and allocation-free beyond the one Security
Package.swiftThe SummaryThe implementation is clean and the happy-path coverage is solid. The two actionable items before merge are:
Everything else is optional polish. |
|
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. |
Closes #117
Summary
Adds
Sources/AiSTKit/AuthenticationMiddleware.swift— apublicOpenAPIClientMiddlewarethat injects the Anthropic authentication headers into every outgoing request:x-api-keywith the stored API keyanthropic-version: 2023-06-01The request is then forwarded to the next handler via
next(request, body, baseURL). The middleware will be wired into the generated ClaudeKit client by theClaudeKitwrapper in #116.Implementation notes
ClientMiddlewaresignature (includesbody: HTTPBody?and returns(HTTPResponse, HTTPBody?)) — the simplified snippet inDocs/skit-analyze-plan.md§I2 predates the current protocol shape.public importforOpenAPIRuntime,HTTPTypes, andFoundation.URLsince the package builds withInternalImportsByDefaultand these types appear in the public conformance.HTTPField.Name(SwiftLintforce_unwrappingrule) viaif leton the always-valid header names.Acceptance criteria
Verification
swift build— cleanswift 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