fix: inject headers into service params#277
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the "a2asrv" REST handlers by integrating HTTP request headers directly into the "context.Context" for each incoming request. This change ensures that critical information, such as authorization tokens, is readily available to downstream service logic and interceptors, addressing a previously identified issue with header propagation. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request injects HTTP headers into service parameters for REST handlers, allowing interceptors to access them via a new withCallContext helper function. However, this introduces a significant security risk by potentially leaking sensitive headers like Authorization and Cookie to untrusted agents, which contradicts the guideline that authorization should be handled at the interceptor level. Additionally, the current implementation of withCallContext unconditionally creates a new CallContext, which shadows any existing context and breaks authentication performed by upstream middleware. It is recommended to refactor this to use an HTTP middleware to centralize context creation, reduce code duplication, and improve maintainability, while also ensuring sensitive headers are filtered and existing context information is preserved.
a2asrv/rest.go
Outdated
| } | ||
|
|
||
| func (h *restHandler) withCallContext(req *http.Request) context.Context { | ||
| ctx, _ := NewCallContext(req.Context(), NewServiceParams(req.Header)) |
There was a problem hiding this comment.
This implementation introduces two security concerns:
-
Sensitive Information Leakage: All HTTP headers from the incoming request are injected into
ServiceParams. These parameters are passed to theAgentExecutorand can be forwarded by thea2aclientto other services. This results in the leakage of sensitive headers likeAuthorizationandCookieto potentially untrusted agents or third-party services. -
Broken Access Control (Authentication Shadowing): Unconditionally calling
NewCallContextcreates a newCallContextthat shadows any existing one in the request context. This resets theUserinformation to an unauthenticated state (Authenticated: false), effectively breaking any authentication performed by middleware higher up in thehttp.Handlerchain.
Remediation:
- Filter the headers before injecting them into
ServiceParamsto exclude sensitive ones (e.g.,Authorization,Cookie,Proxy-Authorization). - Modify
withCallContextto check if aCallContextalready exists in the context and preserve its information (especially theUser) instead of unconditionally creating a new one.
References
- Authorization should be performed by SDK users at the interceptor level, implying that Authorization headers are sensitive and should not be leaked or mishandled.
a2asrv/rest.go
Outdated
|
|
||
| func (h *restHandler) handleSendMessage(rw http.ResponseWriter, req *http.Request) { | ||
| ctx := req.Context() | ||
| ctx := h.withCallContext(req) |
There was a problem hiding this comment.
This change is repeated across all handlers. To improve maintainability and avoid code duplication, consider using an HTTP middleware to inject the CallContext. This would centralize the logic and apply it to all routes handled by this mux.
You could define a middleware:
func withCallContextMiddleware(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
ctx, _ := NewCallContext(r.Context(), NewServiceParams(r.Header))
next.ServeHTTP(w, r.WithContext(ctx))
})
}And then wrap the mux in NewRESTHandler:
// In NewRESTHandler, at the end
// ...
return withCallContextMiddleware(mux)With this middleware in place, all the handle... functions can be reverted to use ctx := req.Context(), as the context from the request will already contain the ServiceParams. This would make the code cleaner and less prone to errors when adding new handlers.
a2asrv/rest_test.go
Outdated
| ctx := t.Context() | ||
| tid := a2a.NewTaskID() | ||
| task := &a2a.Task{ID: tid, ContextID: a2a.NewContextID()} | ||
| store := testutil.NewTestTaskStore().WithTasks(t, task) |
There was a problem hiding this comment.
nit: this setup here is not really necessary, because we're checking the code which gets executed regardless of whether a task exists or not
🤖 I have created a release *beep* *boop* --- ## [1.0.0](v1.0.0-alpha.3...v1.0.0) (2026-03-17) ### Features * implement the new rest error handling ([#282](#282)) ([a3bda30](a3bda30)) * use v2 suffix for module ID and provide compat support ([#270](#270)) ([dd1b6ba](dd1b6ba)), closes [#250](#250) ### Bug Fixes * a2asrv jsonrpc Content-Type ([#265](#265)) ([2568a46](2568a46)) * bugs before going from alpha ([#279](#279)) ([b1f055c](b1f055c)) * GetTaskRequest nil pointer assignment check ([#258](#258)) ([440bb79](440bb79)) * inject headers into service params ([#277](#277)) ([d33f3bd](d33f3bd)), closes [#275](#275) * propagate cancelation signal using task store ([#272](#272)) ([5e1d462](5e1d462)), closes [#245](#245) * regenerate spec and fix returnImmediately ([#284](#284)) ([2eee0b9](2eee0b9)) * task modified after save ([#266](#266)) ([c15febe](c15febe)) * taskupdater result mutable ([#274](#274)) ([6038d92](6038d92)) * update pushsender ([#256](#256)) ([5f7a594](5f7a594)) * use enum values as in the spec ([#261](#261)) ([eb98981](eb98981)), closes [#251](#251) ### Documentation * **a2asrv:** add Example_* test functions for pkg.go.dev documentation ([#262](#262)) ([7888e37](7888e37)) * add example tests a2a ([#240](#240)) ([4fe08a9](4fe08a9)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
fixes #275