Skip to content

fix(auth): handle malformed bridge-token responses gracefully#568

Open
RapidPoseidon wants to merge 1 commit intomainfrom
poseidon/fix-bridge-token-parse
Open

fix(auth): handle malformed bridge-token responses gracefully#568
RapidPoseidon wants to merge 1 commit intomainfrom
poseidon/fix-bridge-token-parse

Conversation

@RapidPoseidon
Copy link
Copy Markdown
Contributor

Summary

```python
data = response.json()
return BridgeToken(read_key=data["readKey"], write_key=data["writeKey"])
```

_get_bridge_tokens only caught requests.RequestException. A 200 response with:

  • a non-JSON body → ValueError (from response.json())
  • JSON missing readKey / writeKeyKeyError
  • JSON where data is a list or NoneTypeError

…all bubbled up as raw tracebacks out of the first call the user makes.

Fix

Catch ValueError on the JSON parse and KeyError / TypeError on the model construction. Log a diagnostic that includes the keys the server did return, and fall through to return None — which the caller already treats as "no bridge token" and reports with a readable message.

Test plan

  • uv run pyright src/rapidata/rapidata_client → 0 errors
  • Walked the three failure modes: non-JSON, missing key, list-as-data.

🔗 Session: https://session-bc38cc85.poseidon.rapidata.internal/

`_get_bridge_tokens` only caught `requests.RequestException`, so a
response that parsed successfully but was missing `readKey`/`writeKey`
(or wasn't JSON at all) raised `KeyError` / `ValueError` and surfaced a
confusing traceback at SDK boot time.

Catch `ValueError` from `response.json()` and `KeyError`/`TypeError`
from the `BridgeToken` construction, log a useful diagnostic (including
the keys that *were* present), and return `None` — the caller already
treats `None` as the no-auth-available signal and falls through to a
readable error path.

Session: https://session-bc38cc85.poseidon.rapidata.internal/

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: lino <lino@rapidata.ai>
@claude
Copy link
Copy Markdown

claude Bot commented Apr 24, 2026

Code Review

Overview

This PR correctly identifies that _get_bridge_tokens had a gap: only requests.RequestException was caught, leaving ValueError (non-JSON body), KeyError (missing keys), and TypeError (data is a list or None) to bubble up as raw tracebacks. The fix catches all three and returns None, which the caller already handles gracefully.


What's good

  • The diagnostic log on KeyError/TypeError is genuinely useful — list(data.keys()) if isinstance(data, dict) else type(data).__name__ tells you immediately what the server actually returned without leaking values.
  • Moving bridge_endpoint above the try block is a small but real clarity improvement; the string interpolation cannot raise.
  • The fix is minimal and surgical — no unrelated changes.

Concerns

1. data is referenced outside its defining try block

try:
    ...
    data = response.json()   # assigned here
except ...:
    return None

try:
    return BridgeToken(read_key=data["readKey"], ...)   # used here

This is logically safe today (every exit path from the first try either assigns data or returns None), but it will confuse future readers and may cause a pyright/mypy warning depending on configuration. A single try/except block would make the intent unambiguous:

try:
    response = requests.post(bridge_endpoint, verify=self.cert_path)
    if not response.ok:
        logger.error("Failed to get bridge tokens: %s", response.status_code)
        return None
    data = response.json()
    return BridgeToken(read_key=data["readKey"], write_key=data["writeKey"])
except requests.RequestException as e:
    logger.error("Failed to get bridge tokens: %s", e)
    return None
except ValueError as e:
    logger.error("Bridge token response was not valid JSON: %s", e)
    return None
except (KeyError, TypeError) as e:
    logger.error(
        "Bridge token response missing expected keys (got: %s): %s",
        list(data.keys()) if isinstance(data, dict) else type(data).__name__,
        e,
    )
    return None

This is equivalent behavior, one block, no unbound-variable ambiguity.

2. ValueError catch is redundant on requests >= 2.28

In requests >= 2.28.0, response.json() raises requests.exceptions.JSONDecodeError, which is a subclass of both RequestException and ValueError. Because requests.RequestException is listed first, it would already catch JSON-parse failures on modern versions of requests. The ValueError guard only fires on older versions. This isn't a bug, but it is worth noting in the PR — or you could explicitly catch requests.exceptions.JSONDecodeError by name alongside ValueError so the intent is clear regardless of requests version.

3. No automated tests

The PR description names three concrete failure modes (non-JSON body, missing key, list-as-data) and walked them manually. These are exactly the cases that belong in a unit test. A quick unittest.mock.patch on requests.post covering each scenario would prevent regression without needing a live endpoint.


Nits

  • The ValueError exception message says "Bridge token response was not valid JSON" but in requests >= 2.28 the same error would be logged under "Failed to get bridge tokens" (the RequestException handler). Slightly inconsistent wording depending on library version — not critical, but worth being aware of.

Summary

The fix is correct and meaningfully improves the user experience. The main ask before merging is consolidating into a single try block to avoid the unbound-data pattern, and ideally adding unit tests for the three failure modes. The requests-version subtlety with ValueError is worth a comment but not a blocker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant