feast-dev / feast-dev/feast

What is _validate_token meant to verify in the OIDC token parser?

Open
#6,688 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
7.3k
Forks
1.4k
Avg merge
1d 21h
Merged PRs (30d)
15

Description

## Question

What is `OidcTokenParser._validate_token` intended to verify? As written it cannot reject any token, but it reads like a security gate and logs as though one passed, so I would rather ask than assume it is dead code.

## What it does today

[`_validate_token`](https://github.com/feast-dev/feast/blob/3a6a1031ac501c715b6d7aaf6d6aa3cee25c5367/sdk/python/feast/permissions/auth/oidc_token_parser.py#L45-L60) builds a mock request, sets `Authorization: Bearer ` on it from the token it was handed, and awaits `OAuth2AuthorizationCodeBearer`. That scheme only parses the header and returns the token; it never inspects the value. Since the method supplies the header itself, the check always succeeds.

Verified against the installed fastapi (0.139.2) — every one of these is accepted, including the empty string:

```
'' -> ACCEPTED ''
' ' -> ACCEPTED ''
'garbage' -> ACCEPTED 'garbage'
'a.b.c' -> ACCEPTED 'a.b.c'
```

The caller then logs success:

```python
await self._validate_token(access_token)
logger.debug("Token successfully validated.")
```

Real verification happens afterwards in `_decode_token`, which fetches the JWKS signing key and calls `jwt.decode`. So the token is genuinely validated, just not here. My concern is that the method name, the docstring ("Validate the token ... against the OAuth2 server"), and that log line together suggest a verification step that does not exist, which is the kind of thing that misleads a future reader auditing the auth path.

## The one thing it does affect

Constructing the scheme reads `token_endpoint` and `authorization_endpoint` from the discovery document, so a discovery document **missing** `token_endpoint` raises a `ValidationError` and the request fails (an empty string passes). That is the only input that can make this method fail.

Worth noting the server-side validation path itself never needs a token endpoint — it only needs `jwks_uri`. So this effectively requires a discovery field that Feast's own token validation does not use. It also does not trigger anything that would otherwise be skipped: `_decode_token` reaches the same lazily-fetched discovery data via `get_jwks_url()`.

## Why I am asking rather than sending a patch

Removing the call would be a behavior change for anyone whose provider omits `token_endpoint` (they would go from failing to working), and it is the only `await` on this branch of the OIDC path, which matters if the blocking JWKS work is ever moved to a thread. Both seem like your call rather than mine.

Roughly the options I see:

1. It is vestigial, and the call plus the misleading log line can go.
2. The discovery-document shape check is wanted, in which case it would be clearer done once at parser construction rather than per request, and against the fields actually required.
3. It was meant to do something else entirely (introspection against the provider?) and never did, in which case that is the real issue.

Happy to send a PR for whichever you prefer. Not urgent, and the per-request cost is small; this is about the auth path saying it does something it does not.

Contributor guide

Open the contributing guide

Research direction

Start with sdk/python/feast/permissions/auth/oidc_token_parser.py, reading _validate_token, its caller, and _decode_token; compare OAuth2AuthorizationCodeBearer's parsing behavior with the discovery fields it loads. Decide which contract the auth path should expose and whether the current discovery-field dependency is intentional. Done means the chosen behavior, method naming, logging, and validation path agree, with the observed empty or garbage-token and missing-token_endpoint cases covered.

Written by the indexing model from the issue text.

Assessment

Tech stack
fastapi, python
Domain
authentication, backend, security
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.