oscerd commented on PR #26974:
URL: https://github.com/apache/camel/pull/26974#issuecomment-5868438075
Thanks — this was a genuinely useful review, and point 2 was a real bypass.
All seven are addressed, plus the JIRA. I verified each one against the live
SDK and a real OpenFGA 1.21.0 before changing anything, so here is the evidence
alongside the fix.
**1. Duplicate dependency — fixed.** My bug, and worse than a stray
copy-paste: the script I used to insert the registration entries had a regex
whose `(?:[^\n]*\n)*?` spanned many blocks, so it matched the *first*
`<dependency>` in `dependencyManagement` (the camel core block) as well as the
correct alphabetical slot. Removed the stray one. I also checked the other
three registration poms in case the same helper had misfired there —
`bom/camel-bom` and `catalog/camel-allcomponents` are correctly placed, and the
two entries in `test-infra/camel-test-infra-all` are intentional (a
`<dependency>` and a `<fileSet>`, as `camel-test-infra-opa` has).
**2. `failOpen` too broad — fixed, and you were right that it was
exploitable.** Confirmed against OpenFGA 1.21.0:
| object (passes `OpenFgaIdentifiers`) | server |
|---|---|
| `document:a:b` | HTTP 400 `invalid 'object' field format` |
| `document:x#y` | HTTP 400 `invalid 'object' field format` |
| `nosuchtype:x` | HTTP 400 `type 'nosuchtype' not found` |
`#` is legitimate in a userset *subject*, so the guards let it through as an
object, and with `failOpen=true` and `object=document:${header.documentId}` a
caller sending `x#y` was allowed.
`failOpen` now applies only when OpenFGA could not answer. The classifier
walks the cause chain for an `FgaError` and uses the SDK's own predicates —
`!isClientError() || isRateLimitError()` (I checked the bytecode:
`isClientError()` is status ∈ [400,500), `isRateLimitError()` is 429 or
`rate_limit_exceeded`). So 4xx fails closed, 429 and 5xx and transport failures
and timeouts still fail open, and when `failOpen` is set but does not apply the
component logs *why* rather than denying silently.
Tests: parameterized over 400/401/403/404 (deny) and 500/502/503 plus 429
(allow), and an IT that first asserts the server really does answer 400 for
`document:x#y` and then that the route is denied anyway. The doc's failOpen
section now states the 4xx carve-out explicitly.
**3. Invalid `consistency` — fixed.** You are right, and my code comment
claimed the opposite of the truth. Measured:
```
fromValue("HIGHER_CONSISTENCY") = HIGHER_CONSISTENCY
fromValue("higher_consistency") = unknown_default_open_api
fromValue("TYPO") = unknown_default_open_api
```
Now rejected at endpoint start with a message listing the three valid
values, the comment is corrected to say what `fromValue` actually does, and
there are tests for both the accepted values and the lowercase typo.
**4. Body replacing the configured tuple — fixed, not just noted.** Agreed
that for this component it was the wrong default. `writeTuples`/`deleteTuples`
now use the endpoint's `user`/`relation`/`object` whenever it names any of
them, and read the body only when it names none. A partly configured triple is
reported as a mistake rather than completed from the message. There is an
IMPORTANT block in "Granting and revoking access" and a test asserting that a
body of
`{"user":"user:attacker","relation":"owner","object":"document:secret"}` does
not override a configured triple.
**5. Docs example — fixed.** Correct: `sql:` yields a `List<Map>` whose
entries stringify to `{ID=budget}`, fail validation, and get skipped, so the
example returned an empty list. It now transforms the rows to `document:<id>`
first, with a comment saying why.
**6. Missing `scopes` — added** next to the rest of the client-credentials
options.
**7. `CamelOpenFgaAllowed` unset when every entry is invalid — fixed** to
`false`, with a test.
**JIRA** — `fixVersions` set to 4.23.0.
90 unit tests and 12 ITs pass; the branch is rebased on current `main` and a
full reactor leaves `git status` clean.
One note for the record rather than a disagreement: this review carries a
`_Claude Code on behalf of davsclaus_` trailer, so by the project's own
AI-attribution convention it is an AI review under a human login. I have not
counted it as the human approval the merge rules require, and I am not
resolving any conversation — please close them off yourself once you have
checked the changes.
---
_Claude Code on behalf of @oscerd_
--
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]
