fix(variables): validate variable keys before writing them - #3155
fix(variables): validate variable keys before writing them#3155levivannoort wants to merge 3 commits into
Conversation
The API now rejects variable keys that are not valid environment variable names, but the console had no key validation anywhere — only a value length check. Keys with hyphens, dots, spaces or a leading digit reached the API and came back as a bare server error, and three flows handled that badly: - create-function and create-site create the resource (and its proxy rule) before the variables, so a rejected key left a function or site with no deployment behind - the update modal resends the stored key on a value-only edit, so a key stored before the rule existed could no longer have its value changed - promote-to-global deletes the existing global variable before creating its replacement, so a rejected key destroyed the original Add a shared validator and check keys before submitting, in every entry point that writes them. Send the key on update only when it changed, so the API keeps the stored one and existing keys stay editable. Flag keys that cannot be used as environment variable names in the variables tables, since those are ignored at build and runtime. Report per-key failures from the batch writes instead of a single rejection. Server-side rule: appwrite/appwrite#13181 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Console (appwrite/console)Project ID: Tip Every Git commit and branch gets its own deployment URL automatically |
Greptile SummaryThis PR adds shared environment-variable key and value validation across creation, editing, and import flows, while improving handling of legacy keys and partial batch failures.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (6): Last reviewed commit: "fix(variables): filter valueless entries..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ); | ||
| } | ||
| }); | ||
| const validationError = validateVariables( |
There was a problem hiding this comment.
Wouldn’t this also validate keys that are already stored? 🤔 If a project contains a legacy key (examplews MY-KEY), updating only its value would fail here before sdkUpdateVariable(..., undefined, ...) is ever reached. Should we only validate keys for newly added variables and keep validating values for existing ones?
cc @greptile
There was a problem hiding this comment.
Good catch, but the concern is scoped to the wrong component. In createVariableModal.svelte, validateVariables is only called on newVariables — the rows the user is currently entering in the form. The pre-existing variables prop is never passed through validation here, so legacy stored keys are safe.
The concern is real in updateVariableModal.svelte though. There, pair is initialized from selectedVar (the stored variable), and validateVariables([pair]) runs unconditionally on submit — it doesn't check whether pair.key actually changed. So if a user opens the update modal on a legacy MY-KEY variable to change only the value, the key validation fires and blocks them before sdkUpdateVariable (with undefined key) is ever reached. That's the breakage the PR description says it fixes, but looking at the actual code in updateVariableModal.svelte the guard isn't there.
The fix would be to skip key validation when the key is unchanged:
function handleVariable() {
const keyChanged = pair.key !== selectedVar?.key;
const toValidate = keyChanged ? pair : { key: undefined, value: pair.value };
const validationError = validateVariables([toValidate]);
...
}But validateVariables → getVariableKeyError returns 'Variable key is required' for an undefined key, so validateVariables would also need to treat key === undefined as "skip key check" (i.e. only validate value). Worth addressing before merge — as-is, updateVariableModal.svelte still breaks value-only edits on any pre-existing invalid key.
There was a problem hiding this comment.
Checked both create modals and both update modals — no change needed here, and the split you describe is what is already implemented.
Create modals (lib/components/variables/createVariableModal.svelte, project-[region]-[project]/createVariableModal.svelte): validateVariables only ever receives newVariables, the rows being typed into the form. The pre-existing variables prop is never passed through it, so stored keys are untouched.
Update modal (project-[region]-[project]/updateVariablesModal.svelte) — this is the one reachable from the post-create variables table, and it already guards on the key having changed:
const keyChanged = pair.key !== originalKey;
const validationError = keyChanged
? validateVariables([pair])
: getVariableValueError(pair.key, pair.value);so a value-only edit on a legacy MY-KEY validates the value only, and dispatches key: undefined so the API keeps the stored key.
lib/components/variables/updateVariableModal.svelte (the singular one, which greptile pointed at) validates unconditionally, but as you noted in the other thread it is only reachable through EnvironmentVariables in the two create-flow configuration components, where the key is always user-entered.
| // the API keep the stored one. | ||
| await sdkUpdateVariable(variable.$id, undefined, newValue, false); | ||
| } | ||
| delete vars[variable.key]; |
There was a problem hiding this comment.
Since this now uses Promise.allSettled, could a failed update leave the key in vars until the second pass? If so, wouldn’t that cause it to be processed again and end up calling sdkUpdateVariablewith the key, which is what this PR is trying to avoid? Would it make sense to move delete vars[variable.key] before the awaited update? 🤔
cc @greptile
There was a problem hiding this comment.
Good catch — the concern is valid. If sdkUpdateVariable or sdkDeleteVariable throws in the first pass, the delete vars[variable.key] line is never reached. The key stays in vars, and the second pass picks it up as an "existing variable" and calls:
await sdkUpdateVariable(existingVariable.$id, key, vars[key], false);…this time with the key argument, which is precisely what the PR is trying to avoid for legacy invalid keys.
Moving the delete before the await alone is slightly risky if the value is needed later, but a try/finally is the clean fix:
| delete vars[variable.key]; | |
| editableVariables.map(async (variable) => { | |
| const newValue = vars[variable.key] ?? null; | |
| try { | |
| if (newValue === null) { | |
| await sdkDeleteVariable(variable.$id); | |
| } else if (newValue !== variable.value) { | |
| // The key is unchanged here, so leave it out and let | |
| // the API keep the stored one. | |
| await sdkUpdateVariable(variable.$id, undefined, newValue, false); | |
| } | |
| } finally { | |
| delete vars[variable.key]; | |
| } |
finally guarantees the key is removed from vars whether the operation succeeds or fails, so the second pass never re-processes an existing key with the key argument. Failures are still captured by existingResults and surfaced in the error summary — nothing is silently swallowed.
There was a problem hiding this comment.
Good catch — you were right, and it was worse than just the key being resent: the failed variable got a second write attempt too. Fixed in 0feeb2a by claiming the key before the awaits rather than after:
const newValue = vars[variable.key] ?? null;
// Claim the key up front. A rejected write must not leave it
// behind for the second pass, which would retry it as a new
// variable and send the stored key along with it.
delete vars[variable.key];I went with moving the delete up rather than try/finally since newValue is already captured in a local and nothing reads vars[variable.key] afterwards, so there is no state to preserve across the await. Failures are still collected by existingResults and named in the error summary.
One note on scope: the delete branch was already safe, since newValue === null means the key is absent from vars and the delete was a no-op. It was only the update branch that could leak.
|
@greptile re-review |
A rejected update left its key in vars, so the second pass picked it up as a new variable and resent the stored key — the exact call the sparse update is meant to avoid for keys that predate the identifier rule.
|
@greptile re-review |
An entry with an empty value is discarded rather than written, so an invalid key on one of them was rejecting the whole file and taking the valid variables down with it.
|
@greptile re-review |
2 participants
What
Adds variable key validation to the console, in every flow that writes one, and fixes three flows that handled a rejected key badly.
Why
appwrite/appwrite#13181 makes the API reject variable keys that are not valid environment variable names (
^[A-Za-z_]\w*$, max 255). The console had no key validation anywhere — only a value length check — so keys with hyphens, dots, spaces or a leading digit reached the API and returned a bare server error.Three flows turn that error into real damage:
create-function/*andcreate-site/*create the function or site (and its proxy rule) before the variables. A rejected key aborts mid-flow and leaves a resource with no deployment.Changes
New
src/lib/helpers/variables.ts—isValidVariableKey,getVariableKeyError,getVariableValueError,validateVariables(+ unit tests). The existingnormalizeDetectedVariables/mergeVariablesare unchanged.Validation wired into all seven entry points: both create modals, both update modals, the
.envimport modal, and both raw editors. The existing 8192-character value check is folded into the shared helper, so each call site keeps one check instead of two.Behaviour fixes:
updateVariablesModalvalidates the key only when it changed and omits it otherwise;handleVariableSecretnever sends it. The endpoint treatskeyas sparse (Update.php:107) and the SDK dropsundefined, so a value-only edit on a pre-existingMY-KEYnow succeeds.sdkUpdateVariable's key parameter widens tostring | undefinedin three components.create()call, so an invalid key can no longer orphan a resource.allSettledand name the keys that failed, instead of surfacing one rejection for the whole batch.Testing
bun run check(0 errors, 87 warnings — unchanged baseline),bun run lint(0 errors),bun run test:unit(243 passed),bun run buildall pass.src/lib/helpers/oauth2-cimd.test.tsfails to load locally on a missingAPPWRITE_ENDPOINT; it arrives from 9dde997 and is untouched here.