Skip to content

feat(api): let settings be read over GET, keep POST for writes - #172

Merged
jherforth merged 1 commit into
jherforth:mainfrom
mrramam:feat/settings-read-over-get
Sep 15, 2026
Merged

jherforth merged 1 commit into
jherforth:mainfrom
mrramam:feat/settings-read-over-get

Conversation

@mrramam

@mrramam mrramam commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

GET /api/settings now accepts ?keys=KEY,PREFIX_* and narrows the read the same way POST /api/settings/search does. Omitting keys returns the whole table, which is what this route has always done, so the change is additive.

The three client callers move to it: the language probe on boot, the Admin Panel (unfiltered, it edits the whole table), and PhotoWidget, which reads its settings on every mount.

POST /api/settings/search stays, unchanged and supported. It is documented in plugin-development.md, so third-party plugins are written against it and removing it would break them. Both routes now share one lookup function rather than two copies of the LIKE-building.

Why bother: a read shaped as a write is not cacheable, and it lands in any audit of settings mutation. That second cost is not hypothetical - PhotoWidget mounts once per tab rotation on a wall display, so this POST fired at the cadence of a real write defect while that defect was being traced, and had to be recognized and set aside by hand.

Tests cover the filter directly: an exact key does not match its siblings, a wildcard does not spill past its prefix, several keys combine, an empty filter means no filter rather than no results, and redaction still applies to the filtered path so WEATHER_API_KEY cannot be pulled out by naming it.

GET /api/settings now accepts ?keys=KEY,PREFIX_* and narrows the read the same
way POST /api/settings/search does. Omitting keys returns the whole table, which
is what this route has always done, so the change is additive.

The three client callers move to it: the language probe on boot, the Admin Panel
(unfiltered — it edits the whole table), and PhotoWidget, which reads its
settings on every mount.

POST /api/settings/search stays, unchanged and supported. It is documented in
plugin-development.md, so third-party plugins are written against it and
removing it would break them. Both routes now share one lookup function rather
than two copies of the LIKE-building.

Why bother: a read shaped as a write is not cacheable, and it lands in any audit
of settings mutation. That second cost is not hypothetical — PhotoWidget mounts
once per tab rotation on a wall display, so this POST fired at the cadence of a
real write defect while that defect was being traced, and had to be recognized
and set aside by hand.

Tests cover the filter directly: an exact key does not match its siblings, a
wildcard does not spill past its prefix, several keys combine, an empty filter
means no filter rather than no results, and redaction still applies to the
filtered path so WEATHER_API_KEY cannot be pulled out by naming it.
@mrramam mrramam changed the title feat(api): let settings be read over GET, keep PUT for writes feat(api): let settings be read over GET, keep POST for writes Sep 14, 2026
@jherforth

Copy link
Copy Markdown
Owner

Reviewed critically and merging. I went after the redaction claim hardest, since that is the one where being wrong is expensive.

Redaction holds, every way I could think of to ask

Against a live server with WEATHER_API_KEY and GOOGLE_CLIENT_SECRET_ENC stored:

?keys=WEATHER_API_KEY        200 | keys=0 | leaks=[]
?keys=WEATHER_*              200 | keys=0 | leaks=[]
?keys=*                      200 | keys=7 | leaks=[]
?keys=%                      200 | keys=7 | leaks=[]
?keys=WEATHER_API_KE_        200 | keys=0 | leaks=[]
?keys=_EATHER_API_KEY        200 | keys=0 | leaks=[]
(no filter)                  200 | keys=7 | leaks=[]
POST /search ["WEATHER_*"]   200 | leaks=[]

Both routes funnel through rowsToSettingsObject, so a secret cannot be named out of the table. ?keys=' OR 1=1 -- returns 200 with zero keys — the keys are bound as parameters, not interpolated.

I also confirmed GET and POST return byte-identical key sets for the same filter, which is the real point of sharing selectSettings.

The client callers work, and I proved the boot-critical one

The language probe in main.jsx runs before render and swallows its own errors, so a break there would be silent. Cleared the stored language, set default_language=es, and loaded the app:

GET /api/settings?keys=default_language
GET /api/settings
any POST /api/settings/search left? false
booted in Spanish (language probe worked): true

Three sharp edges — two of them are yours to keep, one is new

1. _ is a LIKE wildcard, so an "exact" key is not exact. With KEYFILTER_ALPHA and KEYFILTERXALPHA both stored:

?keys=KEYFILTER_ALPHA -> ["KEYFILTER_ALPHA","KEYFILTERXALPHA"]

Your test asserts "an exact key must not match its siblings", but its siblings differ in length so it passes without exercising this. Not introduced herePOST /search ["KEYFILTER_ALPHA"] returns the same two, so the shared function preserves behaviour exactly as intended. It cannot leak a secret (redaction runs after), and no real HomeGlow key set collides. Flagging it only because the docs now present ?keys=KEY as an exact lookup and point new plugins at it. Escaping _ would be a one-liner but would change the documented POST semantics too, so leaving it is defensible.

2. A very long filter 500s. ?keys= with 3001 entries returns 500 (SQLite parameter limit); 900 is fine. Also pre-existingPOST /search with a 3001-element array 500s identically.

3. Repeated keys params silently return everything. This one is new surface, since POST takes an array:

?keys=KEYFILTER_ALPHA&keys=OTHER_GAMMA
  -> all 7 keys

Fastify hands an array for a repeated param, parseSettingsKeysParam sees typeof raw !== 'string' and returns [], which means "no filter". So it fails open to the whole table rather than erroring. Harmless — redaction still applies and the unfiltered read is the documented default — but a caller who writes it that way gets a much larger response than they asked for, which is the sort of thing that turns into a puzzled bug report later. Taking the last value, or joining the array, would close it.

None of these block: the first two are faithfully preserved behaviour and the third is benign.

Verification

Server 225/225, client 281/281, translation parity, build clean, CI green on both jobs.

Agreed on the motivation, too — a read shaped as a write showing up in an audit of settings mutation while chasing a real write defect is a good reason on its own, never mind the caching.

@jherforth
jherforth merged commit 4be7b38 into jherforth:main Sep 15, 2026
2 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in HomeGlow Kanban Sep 15, 2026
@jherforth jherforth added the enhancement New feature or request label Sep 15, 2026
@jherforth jherforth added this to the 1.9 milestone Sep 15, 2026
@mrramam
mrramam deleted the feat/settings-read-over-get branch September 15, 2026 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants