Skip to content

Allow HTTPFetcher to pass through 304 responses#4120

Merged
khassel merged 1 commit intoMagicMirrorOrg:developfrom
sonnyb9:codex/httpfetcher-304-pass-through
Apr 27, 2026
Merged

Allow HTTPFetcher to pass through 304 responses#4120
khassel merged 1 commit intoMagicMirrorOrg:developfrom
sonnyb9:codex/httpfetcher-304-pass-through

Conversation

@sonnyb9
Copy link
Copy Markdown
Contributor

@sonnyb9 sonnyb9 commented Apr 24, 2026

Summary

This updates js/http_fetcher.js so HTTP 304 Not Modified responses are emitted through the normal response event instead of being treated as errors.

That aligns the core fetcher with the built-in yr weather provider in v2.35.0, which already includes explicit logic to handle 304 and reuse cached data.

Closes #4119.

Root cause

HTTPFetcher currently treats all non-OK responses as errors. Since 304 is not an OK response, it never reaches providers listening on the response event.

At the same time, defaultmodules/weather/providers/yr.js expects to receive 304 and handle it by reusing cached data.

What changed

  • special-case HTTP 304 in HTTPFetcher
  • emit 304 through the normal response event
  • preserve the existing error path for other non-OK statuses

Validation

  • confirmed defaultmodules/weather/providers/yr.js already expects response.status === 304
  • compared the unpatched v2.35.0 HTTPFetcher behavior to the provider logic
  • ran node --check js/http_fetcher.js

@rejas
Copy link
Copy Markdown
Collaborator

rejas commented Apr 24, 2026

Please base your PR against the develop branch

Comment thread js/http_fetcher.js Outdated
@sonnyb9 sonnyb9 force-pushed the codex/httpfetcher-304-pass-through branch from e34dcb7 to 4484407 Compare April 25, 2026 15:30
@sonnyb9 sonnyb9 changed the base branch from master to develop April 25, 2026 15:31
@sonnyb9
Copy link
Copy Markdown
Contributor Author

sonnyb9 commented Apr 25, 2026

Thanks for the reply.

Retargeted this PR to develop and force-pushed a rebased branch so the diff stays limited to the HTTPFetcher change.

I also updated the implementation to treat 304 as part of the successful response path, which preserves the existing serverErrorCount / networkErrorCount reset behavior, and added a focused unit test for that case.

Validation on this checkout: node --check js/http_fetcher.js. I also attempted npm test -- --run tests/unit/functions/http_fetcher_spec.js, but vitest is not installed locally in this environment.

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator

KristjanESPERANTO commented Apr 25, 2026

but vitest is not installed locally in this environment.

Once you run npm run install, you can run the tests.

Is there a reason why the PR is still marked as a draft?

@sonnyb9 sonnyb9 marked this pull request as ready for review April 25, 2026 19:21
@sonnyb9
Copy link
Copy Markdown
Contributor Author

sonnyb9 commented Apr 25, 2026

Tests are passing locally now after installing the repo dependencies.

Ran:
./node_modules/.bin/vitest run tests/unit/functions/http_fetcher_spec.js

Result:
Test Files 1 passed (1)
Tests 20 passed (20)

Marked the PR ready for review.

@KristjanESPERANTO
Copy link
Copy Markdown
Collaborator

Looks good, thank! 🙂 Even though there are plenty of clear indications of this, let me ask specifically: This is all Vibe Coded, right?

It’s not a problem for me; I’d just like to have it explicitly stated. Maybe we should include this in our PR requirements, like other projects - specifying how much of the process was handled by AI.

@sonnyb9
Copy link
Copy Markdown
Contributor Author

sonnyb9 commented Apr 25, 2026

Yes, it was coded with codex/gpt-5.4.

@khassel khassel merged commit b8548f2 into MagicMirrorOrg:develop Apr 27, 2026
@khassel khassel mentioned this pull request Apr 30, 2026
khassel added a commit that referenced this pull request Apr 30, 2026
## Release Notes
Thanks to: @cgillinger, @khassel, @KristjanESPERANTO, @sonnyb9
> ⚠️ This release needs nodejs version >=22.21.1 <23 || >=24 (no change
to previous release)

[Compare to previous Release
v2.35.0](v2.35.0...v2.36.0)

This release falls outside the quarterly schedule. We opted for an early
release due to:
- Security fix for the internal cors proxy
- API change of the weather provider smi
- Several bug fixes

### Breaking Changes

The cors proxy is now disabled by default. If required, it must be
explicitly enabled in the `config.js` file. See the
[documentation](https://docs.magicmirror.builders/configuration/cors.html).

### ⚠️ Security

You can find several publicly accessible MagicMirror² instances.

This should never be done. Doing so makes your entire configuration,
including secrets and API keys, publicly visible. Furthermore, it allows
attackers to target the host; this is only prevented beginning with this
release.

Public MagicMirror² instances should always run behind a reverse proxy
with authentication.

### [core]
- Prepare Release 2.36.0 (#4126)
- Allow HTTPFetcher to pass through 304 responses (#4120)
- fix(http-fetcher): fall back to reloadInterval after retries exhausted
(#4113)
- config endpoint must handle functions in module configs (#4106)
- fix replaceSecretPlaceholder (#4104)
- restrict replaceSecretPlaceholder to cors with allowWhitelist (#4102)
- fix: prevent crash when config is undefined in socket handler (#4096)
- fix cors function for alpine linux (#4091)
- fix(cors): prevent SSRF via DNS rebinding (#4090)
- add option to disable or restrict cors endpoint (#4087)
- fix: prevent SSRF via /cors endpoint by blocking private/reserved IPs
(#4084)
- chore: add permissions section to enforce pull-request rules workflow
(#4079)
- update version for develop

### [dependencies]
- update dependencies (#4124)
- chore: update dependencies (#4088)
- refactor: enable ESLint rule "no-unused-vars" and handle related
issues (#4080)

### [modules/newsfeed]
- fix(newsfeed): prevent duplicate parse error callback when using
pipeline (#4083)

### [modules/updatenotification]
- fix(updatenotification): harden git command execution + simplify
checkUpdates (#4115)
- fix(tests): correct import path for git_helper module in
updatenotification tests (#4078)

### [modules/weather]
- fix(weather): use nearest openmeteo hourly data (#4123)
- fix(weather): avoid loading state after reconnect (#4121)
- weather: fix UV index display and add WeatherFlow precipitation
(#4108)
- fix(weather): restore OpenWeatherMap v2.5 support (#4101)
- fix(weather): use stable instanceId to prevent duplicate fetchers
(#4092)
- SMHI: migrate to SNOW1gv1 API (replace deprecated PMP3gv2) (#4082)

### [testing]
- ci(actions): set explicit token permissions (#4114)
- fix(http_fetcher): use undici.fetch when dispatcher is present (#4097)
- ci(codeql): also scan develop branch on push and PR (#4086)
- refactor: replace implicit global config with explicit global.config
(#4085)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: sam detweiler <sdetweil@gmail.com>
Co-authored-by: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com>
Co-authored-by: Veeck <github@veeck.de>
Co-authored-by: veeck <gitkraken@veeck.de>
Co-authored-by: Magnus <34011212+MagMar94@users.noreply.github.com>
Co-authored-by: Ikko Eltociear Ashimine <eltociear@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: DevIncomin <56730075+Developer-Incoming@users.noreply.github.com>
Co-authored-by: Nathan <n8nyoung@gmail.com>
Co-authored-by: mixasgr <mixasgr@users.noreply.github.com>
Co-authored-by: Savvas Adamtziloglou <savvas-gr@greeklug.gr>
Co-authored-by: Konstantinos <geraki@gmail.com>
Co-authored-by: OWL4C <124401812+OWL4C@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+bughaver@users.noreply.github.com>
Co-authored-by: BugHaver <43462320+lsaadeh@users.noreply.github.com>
Co-authored-by: Bugsounet - Cédric <github@bugsounet.fr>
Co-authored-by: Koen Konst <koenspero@gmail.com>
Co-authored-by: Koen Konst <c.h.konst@avisi.nl>
Co-authored-by: dathbe <github@beffa.us>
Co-authored-by: Marcel <m-idler@users.noreply.github.com>
Co-authored-by: Kevin G. <crazylegstoo@gmail.com>
Co-authored-by: Jboucly <33218155+jboucly@users.noreply.github.com>
Co-authored-by: Jboucly <contact@jboucly.fr>
Co-authored-by: Jarno <54169345+jarnoml@users.noreply.github.com>
Co-authored-by: Jordan Welch <JordanHWelch@gmail.com>
Co-authored-by: Blackspirits <blackspirits@gmail.com>
Co-authored-by: Samed Ozdemir <samed@xsor.io>
Co-authored-by: in-voker <58696565+in-voker@users.noreply.github.com>
Co-authored-by: Andrés Vanegas Jiménez <142350+angeldeejay@users.noreply.github.com>
Co-authored-by: cgillinger <christian.gillinger@gmail.com>
Co-authored-by: Sonny B <43247590+sonnyb9@users.noreply.github.com>
Co-authored-by: sonnyb9 <sonnyb9@users.noreply.github.com>
khassel pushed a commit that referenced this pull request Apr 30, 2026
After #4120 was merged, here's a bit of polish on top.

- docs: Optimize JSDoc and comments.
- test: The existing 304 test is moved to the dedicated status code
block and extended with an `errorSpy` to also verify that no `error`
event is emitted.
- refactor: The success branch is moved before the error branch in
`fetch()`. This is more intuitive.

The bottom line is that this PR improves maintainability. For users,
this PR doesn't change anything.
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.

HTTPFetcher drops HTTP 304 responses needed by yr provider cache handling

4 participants