Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API or route to produce CSS resource #1097

Open
ericscheid opened this issue Nov 10, 2020 · 21 comments · May be fixed by #3075
Open

API or route to produce CSS resource #1097

ericscheid opened this issue Nov 10, 2020 · 21 comments · May be fixed by #3075
Labels
feature P2 - minor feature or not urgent Minor bugs or less-popular features solution found A solution exists; just needs to be applied

Comments

@ericscheid
Copy link
Collaborator

ericscheid commented Nov 10, 2020

Due to some (many?) browsers being particularly persnicketty about cross-domain requests, it's getting increasingly harder to reference a CSS style sheet resource from a 3rd party domain.

This would normally be done with

<style>
   @import url("https://3rdparty.com/my_wonderful_brew.css");
</style>

However, unless that resource is hosted by a server that knows to issue the correct media-type, the receiving browser will reject due to cross-domain concerns.

What would be handy would be for a HomeBrewery user is that they create a HB document into which they type all their CSS code (without wrapping with <style> and </style>), and then in their other brews they can write an @import to the same-domain resource (e.g. @import url("/css/mumblecode").

The HB server mostly only needs to respond to /css/:id requests with just the brew content, no metadata, no UI chrome; and with the http header of Content-Type: text/css. It probably should do some sanitizing, just in case.

@ericscheid
Copy link
Collaborator Author

Hmm .. what are the security issues here? Could someone craft a particular CSS resource, attach it via a brew via @import, and then publish the /share/:id for that brew and somehow hurt visitors to the that shared brew?

Can JS be embedded inside CSS, for example?

@ericscheid
Copy link
Collaborator Author

@calculuschild
Copy link
Member

calculuschild commented Nov 10, 2020

I don't know... A separate CSS page was something @stolksdorf had originally planned and it would be nice to have. We already allow custom CSS directly within the main brew text, so I don't know that allowing a separate CSS import from a Homebrewery document would be any worse. We can prevent loading if the browser is IE. We can sanitize other input to some extent.

What do you think? If this would be a security issue, it seems we already have this problem because people could make a malicious document and share it via the existing share/:id links, right?

In any case, I think I want to keep this issue open. I think this could benefit from more discussion and research before we definitively say it's off the table.

@calculuschild calculuschild reopened this Nov 10, 2020
@ericscheid
Copy link
Collaborator Author

Good points. I was assuming the render of the brew is sanitized, including any <style> elements — the basic proposal here was for the raw brew to be served out. That doesn't have to be the case — the brew content could be sanitized on the /css/:id route too.

I've not tested what happens with @import url(/source/:id) .. there's a hidden bit of html served up that might confuse browsers into thinking it isn't actually css.

@Gazook89
Copy link
Collaborator

I'm all for this, but have nothing more to contribute than that...Was just searching to find out why a .css file hosted on my github was not loading on homebrewery.

@ericscheid
Copy link
Collaborator Author

Quite some time has passed, and we now have a separate editor pane for CSS.

The contents of which are stored however in the same mongo document as the brew contents, not a separate document.

I think a /css/:id route for returning only the css part of a brew would still be helpful for shared/reusable css stylesheets (for the reasons above, i.e. avoiding cross-domain requests).

I would suggest though that it only returns the css document part, and not scan thru the brew for any <style/> chunks to include. That way a standardised stylesheet can be used (and re-used) without lumbering it with any brew-specific css. Plus, much simpler to code.

@5e-Cleric
Copy link
Member

I would love to be able to have common CSS between files, and be able to edit them all at the same time while watching the brew.
I realise this sounds very tricky, just wanted to throw it in.

@ericscheid
Copy link
Collaborator Author

ericscheid commented Oct 1, 2023

Probably not that tricky.

This is how /download/:id is handled, and /css/:id would just be a variant of this, changing just line 205 and line 208.

homebrewery/server/app.js

Lines 188 to 209 in 52dcc3b

//Download brew source page
app.get('/download/:id', asyncHandler(getBrew('share')), (req, res)=>{
const { brew } = req;
sanitizeBrew(brew, 'share');
const prefix = 'HB - ';
const encodeRFC3986ValueChars = (str)=>{
return (
encodeURIComponent(str)
.replace(/[!'()*]/g, (char)=>{`%${char.charCodeAt(0).toString(16).toUpperCase()}`;})
);
};
let fileName = sanitizeFilename(`${prefix}${brew.title}`).replaceAll(' ', '');
if(!fileName || !fileName.length) { fileName = `${prefix}-Untitled-Brew`; };
res.set({
'Cache-Control' : 'no-cache',
'Content-Type' : 'text/plain',
'Content-Disposition' : `attachment; filename*=UTF-8''${encodeRFC3986ValueChars(fileName)}.txt`
});
res.status(200).send(brew.text);
});


That, and adding some brief documentation into

homebrewery/changelog.md

Lines 80 to 81 in 52dcc3b

## changelog
For a full record of development, visit our [Github Page](https://github.com/naturalcrit/homebrewery).

@ericscheid ericscheid added solution found A solution exists; just needs to be applied P2 - minor feature or not urgent Minor bugs or less-popular features labels Oct 1, 2023
@G-Ambatte
Copy link
Collaborator

My first thought is that if the two are that similar, we might be able to create a unified function to avoid code duplication.

@G-Ambatte G-Ambatte linked a pull request Oct 1, 2023 that will close this issue
@calculuschild
Copy link
Member

calculuschild commented Oct 3, 2023

I think a /css/:id route for returning only the css part of a brew would still be helpful for shared/reusable css stylesheets (for the reasons above, i.e. avoiding cross-domain requests).

This is stepping on the toes of Custom Themes, which is already designated as the preferred approach to common CSS. Adding a /css route and using @import raises the question of potentially dangerous transclusion issues. From Gitter:

We would want to ensure it's done in a way that doesn't trigger massive api calls. I could see a lot of potential grief from users including multiple nested layers of brews/circular dependencies that trigger dozens of brew fetches with each view.
For instance with custom user Themes, we will likely only allow one level of theme nesting (my brew imports a User theme, which cannot itself import a second User theme.) - calculuschild

@ericscheid
Copy link
Collaborator Author

The browser will resolve circular dependencies via caching. 😁

Themes are excellent, though they are a sledgehammer solution. Sometimes an author just wants something lighter, some common css bits and bobs. For example, I'd want to copy in some simple css for easily doing better <dl> stuff in whichever theme I'm currently using.

@5e-Cleric
Copy link
Member

Sometimes an author just wants something lighter, some common css bits and bobs.

You're talking about custom snippets, #1722.

@ericscheid
Copy link
Collaborator Author

I have a mish mash of tweaks I add to brews, usually as I need them, to align with a preferred house style. Things like boxed text, nested lists using disc/circle/square bullets, nested lists not being so heavily indented, blockquotes, and more I've forgotten I've written (usually until about 5 minutes after re-inventing the wheel, yet again).

The pain I'm identifying is having to add in each custom snippet into my Style Editor tab for each new brew as the need is identified, along with knowing that if I tweak or fix for one brew I would need to go back and edit many multiple older brews. That and finding the older brew in which I did indeed already solve the problem so I can then copy/paste the Style Editor snippet. Was it this brew? Or does this one have the up to date version?

@calculuschild
Copy link
Member

calculuschild commented Oct 5, 2023

I may be missing something, but this sounds like the exact use case for Custom Themes.

@ericscheid
Copy link
Collaborator Author

I may be missing something, but this sounds like the exact use case for Custom Themes.

Not quite. I want to extend standard themes with a small set of tweaks and extensions.

Unless custom themes are designed to layer over the top of an existing theme, and not completely swap in replace? That is, I can have one custom theme for my house extensions, and then apply that to brews that use the PHB or DMG (or whatever) theme as their base.

@Gazook89
Copy link
Collaborator

Gazook89 commented Oct 5, 2023

Yeah, custom themes are built with a hierarchy. That’s why there is a Base theme, which PHB builds off of. You can see it in the themes folder…a json that includes a property whose value indicates which existing theme you are appending to.

@ericscheid
Copy link
Collaborator Author

I thought the built-in hierarchy of built-in themes is a separate thing from author managed custom themes.

From the author's p.o.v., they select select a built-in theme (with the built-in hierarchy all managed behind the scenes, it's just code), and optionally also specify a share :id of a brew to use as a custom theme extension, which gets transcluded into the source such that it is applied after the selected brew theme but before the brew style editor css. Is that how it works? If I specify a custom theme it doesn't unload the underlying main theme (and it's base theme)?

@calculuschild
Copy link
Member

calculuschild commented Oct 6, 2023

The way it is designed now, a custom theme would be a brew layered onto an HB theme. So you have:

  1. blank theme
  2. base theme (PHB)
  3. selected HB main theme (DMG)
  4. custom theme stuff (lives in the style tab)

2 and 3 can be skipped if the user wants to build a whole theme from scratch using only the blank theme.

Then when another user wants to use that custom theme in another brew, they optionally select an HB theme, or select the shareId of the custom Theme, which brings with it it's inherited base themes, so the new brew now has:

  1. blank theme
  2. base theme (PHB)
  3. selected HB main theme (DMG)
  4. custom theme from other brew
  5. current brew style tab

with 1-4 baked together from the custom theme.

@calculuschild
Copy link
Member

So Custom themes are inherently tied to a base theme and a main theme. You want to be able to import custom CSS that isn't tied to any specific base theme?

@ericscheid
Copy link
Collaborator Author

So Custom themes are inherently tied to a base theme and a main theme. You want to be able to import custom CSS that isn't tied to any specific base theme?

I'd want to supply a reference to a custom collection of css rules that get applied to a document after that document's base theme has been applied, but before that document's brew.style. So .. I could then import the same custom CSS into brews that use a different base theme. This is particularly useful for e.g. PHB theme vs DMG theme, which are nearly identical .. othewise I'd need to create a "Custom Theme (PHB version)" alongside a "Custom Theme (DMG version)" [etc].

@dbolack-ab
Copy link
Collaborator

Due to some (many?) browsers being particularly persnicketty about cross-domain requests, it's getting increasingly harder to reference a CSS style sheet resource from a 3rd party domain.

This would normally be done with

I think the User Brew-based themes solve for this problem. Where it doesn't/can't, we could look at using the same code that #3501 uses to abstract proxy CSS loading.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature P2 - minor feature or not urgent Minor bugs or less-popular features solution found A solution exists; just needs to be applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants