Skip to content

fix cors proxy getting binary data (e.g. png, webp)#4030

Open
khassel wants to merge 2 commits intoMagicMirrorOrg:developfrom
khassel:cors
Open

fix cors proxy getting binary data (e.g. png, webp)#4030
khassel wants to merge 2 commits intoMagicMirrorOrg:developfrom
khassel:cors

Conversation

@khassel
Copy link
Collaborator

@khassel khassel commented Feb 7, 2026

fixes #3266

@KristjanESPERANTO
Copy link
Collaborator

Are you sure the try-catch will work? Since the response stream is consumed after arrayBuffer(), I think this may suffice:

const arrayBuffer = await response.arrayBuffer();
res.send(Buffer.from(arrayBuffer));

Looking ahead architecturally: We could transition away from the CORS proxy approach mid-term. I'm currently migrating the weather module (which also uses the cors API) to server-side.

Once that's done, we could migrate newsfeed's showFullArticle feature to fetch and parse articles server-side (extracting just the content, like reader mode) instead of embedding full pages in iframes. Then we could remove the cors API entirely, I think.

What do you think?

@jimbo95-beep

This comment was marked as spam.

@khassel
Copy link
Collaborator Author

khassel commented Feb 7, 2026

@KristjanESPERANTO added your suggestion, thanks!

We could transition away from the CORS proxy approach mid-term.

I don't think so because I found a solution using the cors proxy to avoid publishing secrets in browser only modules. Will provide another PR for this, it solves a problem where you have a map url with an access token e.g. in MMM-Flights and hopefully in MMM-RAIN-MAP too ...

@khassel
Copy link
Collaborator Author

khassel commented Feb 7, 2026

Are you sure the try-catch will work?

yes, I needed this for the unit tests ...

@KristjanESPERANTO
Copy link
Collaborator

I don't think so because I found a solution using the cors proxy to avoid publishing secrets in browser only modules.

Interesting, I hadn't thought about third-party modules. That certainly sounds interesting 👍

yes, I needed this for the unit tests ...

Ah, I didn't think about the tests. Would it be possible to update the test mocks from text() to arrayBuffer()? Then we could simplify this part here.

@khassel
Copy link
Collaborator Author

khassel commented Feb 7, 2026

Ah, I didn't think about the tests. Would it be possible to update the test mocks from text() to arrayBuffer()? Then we could simplify this part here.

I already needed 2 hours to get the tests running so feel free to push such a solution on this branch ...

@KristjanESPERANTO
Copy link
Collaborator

I know that struggle 🤯

I can't dig deeper into it this tonight. But I'll take a look at it tomorrow.

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.

3 participants