Remove client-side API key format validation - #1752
Remove client-side API key format validation#1752devin-ai-integration[bot] wants to merge 2 commits into
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🦋 Changeset detectedLatest commit: c9b7f3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Package ArtifactsBuilt from 20f3127. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.45.1-devin-1787328427-drop-api-key-validation.0.tgzCLI ( npm install ./e2b-cli-2.17.2-devin-1787328427-drop-api-key-validation.0.tgzPython SDK ( pip install ./e2b-2.45.1+devin.1787328427.drop.api.key.validation-py3-none-any.whl |
There was a problem hiding this comment.
TASTE.md review: no violations found (0).
Checked the principles this diff can touch: T-52 (client-side validation never mirrors backend rules), T-49/T-50 (ConnectionConfig precedence and E2B_ env vars), T-1/T-1a/T-2 and T-10 (JS ↔ sync/async Python parity of option names), T-16 (ApiParams TypedDict), T-22/T-23 (named Opts types), T-6 (static-by-id forwarding the whole connection surface via SandboxApiOpts), T-54 (flat entry point re-exports), T-57/T-59 (error hierarchy).
The removal is exactly what T-52 asks for: the e2b_+hex regex was a duplicated backend business rule that would drift the moment key issuance changes, and self-hosted deployments issuing other key shapes were being rejected client-side. The validateApiKey / validate_api_key option and E2B_VALIDATE_API_KEY are dropped symmetrically across JS, Python, and the Pick<> in SandboxApiOpts, so parity (T-1, T-10) and the option-forwarding surface (T-6) stay intact; the remaining "API key is required" check still raises AuthenticationError/AuthenticationException (T-57), which is the (c) case T-52 sanctions. No dangling references to the removed names remain in the repo, and AuthenticationError/AuthenticationException imports are still used by other call sites.
One non-TASTE note, not tied to a changed line: the changeset marks this minor for both packages, but removing the public validateApiKey / validate_api_key option is source-breaking for anyone passing it (TypeScript compile error, Python TypeError). Worth confirming that matches the repo's release conventions.
There was a problem hiding this comment.
LGTM — clean, well-scoped removal of the client-side API key format check, mirrored consistently across JS and Python, with a changeset. The one nit found (deleted test files also dropped unrelated 'API key required' coverage) is minor and doesn't block merge.
Extended reasoning...
Overview
This PR removes the client-side e2b_+hex API key format validation (validateApiKey/validate_api_key) from both the JS and Python SDKs, per the updated TASTE T-52 rule that the backend should be the sole trust boundary for business-rule-style checks. It deletes the validation function, the validateApiKey/validate_api_key option from ConnectionOpts/ApiParams, the E2B_VALIDATE_API_KEY env var handling, and the associated dedicated test files, while leaving the unrelated "API key is required" constructor guard untouched in both languages. A changeset is included and documents the breaking behavior change (invalid-format keys now surface as a 401 from the API instead of a local AuthenticationError).
Security risks
None introduced. This shifts an input-format check from client to server, but the server was already the actual authority on key validity — the client-side check was purely a fail-fast convenience, and per the description the backend continues to reject invalid keys via 401. I confirmed via grep that no other references to validateApiKey/validate_api_key/E2B_VALIDATE_API_KEY remain anywhere else in the repo (CLI, docs, other packages), so the removal is self-contained and consistent across the JS and Python surfaces as required by this repo's CLAUDE.md guidance.
Level of scrutiny
Moderate: this does touch public SDK surface (removing an optional constructor/config parameter) and is a breaking change for any caller explicitly passing validateApiKey/validate_api_key: false, which is why the bug-hunting pass flagged the minor-vs-major changeset classification as worth checking — that was examined and ruled out as not a real issue. The change itself is small, mechanical, symmetric across both SDKs, and matches an explicit, already-agreed-upon design policy (TASTE T-52) rather than introducing a new one, which lowers the bar relative to a novel design decision.
Other factors
The only finding from this run is a nit: deleting the two test files wholesale also removed coverage for the still-present, unrelated requireApiKey/"API key is required" constructor guard, which is not otherwise covered elsewhere in the test suite. That's a real but low-stakes regression-coverage gap (production behavior is unaffected) and doesn't rise to the level of blocking approval.
| ) | ||
| } | ||
|
|
||
| if (config.apiKey && config.validateApiKey) { | ||
| validateApiKey(config.apiKey) | ||
| } | ||
|
|
||
| this.api = createClient<paths>({ | ||
| baseUrl: config.apiUrl, | ||
| fetch: createApiFetch(config.proxy), |
There was a problem hiding this comment.
🟡 Deleting packages/js-sdk/tests/api/validateApiKey.test.ts and packages/python-sdk/tests/test_validate_api_key.py wholesale also removed tests for the still-existing, unrelated 'API key is required'/requireApiKey guard in the ApiClient constructor (JS: 'throws when no API key is supplied', 'does not require an API key when requireApiKey is false'; Python: test_api_client_requires_api_key). No remaining test file covers this path — move those specific tests to a surviving file (e.g. connectionConfig.test.ts/test_connection_config.py) instead of dropping them with the rest of the file.
Extended reasoning...
Both deleted test files bundled two unrelated concerns: API-key-format validation (the thing this PR intentionally removes) and the constructor's requireApiKey/'API key is required' guard (which the PR does not touch and which still exists after the diff, at packages/js-sdk/src/api/index.ts:83-89 and packages/python-sdk/e2b/api/__init__.py's ApiClient.__init__). Because the PR deletes the whole files rather than just the format-validation tests within them, it silently drops regression coverage for the unrelated, still-present behavior.
Concretely, packages/js-sdk/tests/api/validateApiKey.test.ts contained describe('ApiClient API key requirement') with two tests: 'throws when no API key is supplied' (asserts new ApiClient(config) throws AuthenticationError matching /API key is required/ when E2B_API_KEY is unset) and 'does not require an API key when requireApiKey is false' (asserts new ApiClient(config, { requireApiKey: false }) does not throw). Neither test exercises validateApiKey/the format regex at all — they test the guard just above the deleted hunk in packages/js-sdk/src/api/index.ts. Likewise, packages/python-sdk/tests/test_validate_api_key.py contained test_api_client_requires_api_key, which asserts ApiClient(config) raises AuthenticationException matching /API key is required/ when E2B_API_KEY is unset — again testing behavior orthogonal to key-format validation.
Grepping both tests/ trees post-PR for requireApiKey/require_api_key/'API key is required' turns up zero matches in any test file — only the source files that implement the guard. The remaining handleApiError.test.ts only covers the HTTP-response 401 mapping (a distinct code path from this client-side constructor check), and Python's test_logging_option.py only constructs ApiClient with a valid key for logging purposes, never exercising the missing-key or requireApiKey: false paths. So the coverage gap is real and not compensated elsewhere.
Step-by-step proof of the gap: (1) before the PR, running the JS suite would fail loudly if someone accidentally removed the if ((opts.requireApiKey ?? true) && !config.apiKey) throw ... check in ApiClient's constructor, because 'throws when no API key is supplied' would fail. (2) After this PR, that same accidental removal would pass the full test suite silently, since no test anywhere asserts that a missing API key throws. (3) The same argument applies symmetrically to the Python ApiClient.__init__ guard and test_api_client_requires_api_key.
This is a real but low-stakes regression: production behavior is completely unchanged by the PR (the guards themselves are untouched and correct), so nothing breaks for users today — this is purely a loss of regression-test coverage that could let a future refactor silently break the 'API key is required' error path. The fix is straightforward: before merging, move the 'ApiClient API key requirement' describe block (JS) and test_api_client_requires_api_key (Python) into a surviving test file — e.g. packages/js-sdk/tests/connectionConfig.test.ts or a new packages/js-sdk/tests/api/apiClient.test.ts, and packages/python-sdk/tests/test_connection_config.py or similarly — rather than deleting them along with the format-validation tests.
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
dupe of #1751 |
Summary
The updated TASTE T-52 rule makes the backend the only trust boundary: client-side validation may exist only to prevent security footguns, to turn opaque transport failures into actionable errors, or to protect type-system bypassers — never to mirror backend business rules. The
e2b_+ hex API key format check mirrored a backend rule (and drifts the moment key formats change, e.g. self-hosted deployments), so it is removed entirely in JS and Python (sync + async shareConnectionConfig):validateApiKeyfrompackages/js-sdk/src/api, thevalidateApiKeyoption onConnectionOpts/SandboxApiOpts, and theE2B_VALIDATE_API_KEYenv var handling.validate_api_keyfrome2b.api, thevalidate_api_keyoption onApiParams/ConnectionConfig, and the env var handling.Breaking for callers passing
validateApiKey/validate_api_keyexplicitly; invalid keys now surface as the API's 401 instead of a client-side error.Link to Devin session: https://app.devin.ai/sessions/1921bb3818604f3a95b0db1272cff3cd
Requested by: @mishushakov