membphis commented on PR #14030: URL: https://github.com/apache/apisix/pull/14030#issuecomment-6409307092
**[P2] Preserve ID-token precedence for explicitly suppressed claims** In [`upstream_header_values`](https://github.com/apache/apisix/blob/094b75a04d3b79649f3cea45f54346363771c6c4/apisix/plugins/openid-connect.lua#L1238-L1242), the claim is serialized before deciding whether the source contains it. Since `serialize_claim(false)` and `serialize_claim(null)` return `nil`, an explicitly present claim is treated as missing, allowing UserInfo to override it. For example, configure: ```json {"header": "X-Admin", "path": ["admin"]} ``` An authenticated request to `GET /reports/admin/export` has these verified claim sources: - ID token: `{"sub": "alice", "admin": false}` - UserInfo: `{"sub": "alice", "admin": true}` The current implementation forwards `X-Admin: true`. Under the documented contract, the ID token takes precedence because it contains `admin`; since its value is `false`, the upstream should receive no `X-Admin` header. The same fallback problem occurs with an explicit `null`. This requires conflicting verified sources, which can occur when claim mappings or data freshness differ. If the upstream relies on this header for authorization, selecting the fallback value can change the authorization outcome despite the documented source priority. I reproduced the `false -> true` result using the helper added by this PR. Please distinguish **claim presence** from **whether the claim produces a header**: inspect the raw lookup result first, select the first source where it is not `nil`, then serialize it and stop searching subsequent sources even if serialization returns `nil`. Please also add regression tests covering: 1. ID token `admin=false`, UserInfo `admin=true`: no mapped header. 2. ID token `admin=null`, UserInfo with a serializable value: no mapped header. 3. Claim absent from the ID token: UserInfo fallback still works. 4. A client-supplied `X-Admin: true` is cleared in the suppressed-value cases. Could you fix the source selection and add these tests before merging? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
