bamaer commented on PR #8335: URL: https://github.com/apache/hop/pull/8335#issuecomment-5652128209
## Overall Good, well-documented work. The `@HopServerServlet(requiredPermission=...)` → overlay → filter chain is the right shape: default-deny is preserved, built-ins can't be shadowed by exact path, the overlay is concurrent, and `findPlugin`'s prefix dispatch deliberately mirrors the mapper's `longestMatch`. The null-`pathInfo` fix and the 503-instead-of-500 on an uninitialized registry are straight improvements. Two things I'd want fixed before merge, plus some coverage and hardening. ## Blocking **1. Plugin paths nested under a built-in prefix get the built-in's weaker permission** In `HopServerEndpointPermissionMapper`, `register()` checks for a built-in collision with an **exact** `get()`, but `requiredPermission()` resolves built-ins by **longest prefix** and returns early on any hit. A plugin at `/hop/status/custom` declaring `run.execute` is therefore accepted into the overlay, resolves to `/hop/status` → `FILE_VIEW`, and still dispatches to the plugin — so a `READ_ONLY` user reaches an endpoint that asked for `run.execute`. Latent today (no plugin does this), but it's the exact failure mode default-deny exists to prevent. Fix: take the longest match across both tables, built-in winning only on equal key length; and have `requirePluginPath` reject a path already covered by a built-in prefix. Worth a test asserting `/hop/status/custom` doesn't resolve to `FILE_VIEW`. **2. Unauthenticated Bearer triggers a fresh JWKS fetch per request** `HopBearerSupport.authenticate` falls through to `validateIdToken`, which builds `new RemoteJWKSet<>(...)` per call (`HopOidcClient.java:184`) — discovery is cached, the JWKS source is not, so its cache is discarded every time. That path used to run once per interactive login; now any unauthenticated client can drive it with a junk `Authorization: Bearer a.b.c` header and turn each request into an outbound fetch to the IdP. Cache the `JWKSource` per `jwks_uri` alongside `DISCOVERY_CACHE`. ## Should fix before merge **3. No tests on the new auth surface.** `HopBearerSupport`, the `HopBasicAuthFilter` bearer branch, the `HopOidcAuthFilter` bearer branch, and `JdbcTokenServlet` have zero coverage. The token service and mapper are tested well; the code that decides *who you are* is not. At minimum: bearer accepted/rejected in BASIC mode, bearer does not create an `HttpSession`, explicit `Authorization` beats an ambient session, garbage bearer → 401 with `WWW-Authenticate`. **4. `validateIdToken`'s null-tolerant checks are now attacker-facing.** Both the issuer and audience checks are skipped when the claim is absent (`claims.getIssuer() != null &&`, `aud != null && !aud.isEmpty() &&`). Defensible when the token came from the token endpoint over TLS; not when it's request-supplied. Require non-null `iss` and `aud` on the bearer path. **5. No revocation story.** A ~1h token survives Log off / force-reauth (`principalFromAuthorization` now accepts Bearer in the force-reauth branch too) and carries roles frozen at issue time, so a role reduction or account removal doesn't take effect for up to an hour. Rotating `HOP_WEB_JDBC_TOKEN_SECRET` is the only lever and it's global. Acceptable for a first cut, but it should be stated explicitly in the docs and ideally in the clipboard message. **6. Secret file permissions.** `loadOrCreateFileSecret()` writes the HMAC key through `HopVfs.getOutputStream` with default umask. Anyone who can read `~/.hop/security/jdbc-token.secret` can mint tokens for any user with any roles. Worth a `0600` attempt on local files, or at least a startup warning if it's group/world-readable. -- 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]
