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

Add camomile #1

Merged
merged 22 commits into from
Oct 5, 2023
Merged

Add camomile #1

merged 22 commits into from
Oct 5, 2023

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Sep 15, 2023

As discussed in the RFC unifiedjs/rfcs#9, here is the implementation of camomile, our Node.js image proxy to work together with rehype-github-image.

How it works

camomile works together with rehype-github-image, which does the following at build time:

  1. The original URL in the content is parsed.
  2. An HMAC signature of the URL is generated.
  3. The URL and HMAC are encoded.
  4. The encoded URL and HMAC are placed into the expected format, creating the signed URL.
  5. The signed URL replaces the original image URL.

After your web app serves the content to the user, camomile takes over:

  1. The client requests the signed URL from camomile.
  2. camomile validates the HMAC, decodes the URL, then requests the content from
    the origin server and streams it to the client.

Features

  • A maxSize option in bytes to limit the downloading of resources (failing if exceeded)
  • Security. Only HTTP(s) is allowed, no other protocols. Protection built-in for Server-Side Request Forgery.
  • Filtering of headers. Request headers from the client which we want to pass on to the remote server and response headers from the remote server to the client.

Future additions

  • Allow audio and video content through options
  • Respect forwarded headers
  • Structured logging (with log levels)
  • Add events (server.on('foo', () => {}))
  • Compression

Time spent

36 hours (without implementing feedback)

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super exciting! I’ve pinged the camomile npm owner again!

First part of nits :)

.github/workflows/main.yml Show resolved Hide resolved
.prettierrc.json Outdated Show resolved Hide resolved
.remarkrc.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Outdated Show resolved Hide resolved
lib/constants.js Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@Murderlon
Copy link
Member Author

Murderlon commented Sep 18, 2023

I think this is ready now. All open comments have a response from me now. There are some comments which mostly seem to be subjective aesthetics / nits, which I wouldn't make blocking for merge but willing to discuss in each thread. There is the comment about rounding down versions numbers, which I find odd but willing to change of course if that is considered blocking.

lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/safe-http-client.js Show resolved Hide resolved
lib/safe-http-client.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
lib/server.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
@wooorm
Copy link
Member

wooorm commented Oct 4, 2023

I think this is good to go now @Murderlon? Do you want to merge this, shall I do it?
Can I help you with the invoice? (Perhaps this can be discussed on the rfc?)

@Murderlon Murderlon merged commit 9183ca5 into main Oct 5, 2023
@Murderlon Murderlon deleted the v0 branch October 5, 2023 07:24
@wooorm
Copy link
Member

wooorm commented Nov 20, 2023

we got the npm name, released! https://github.com/rehypejs/camomile/releases/tag/1.0.0

@JounQin
Copy link
Member

JounQin commented Nov 20, 2023

Maybe a stupid question, as a non-native English person, I don't quite get why camomile is chosen to be the package name? How does it relate to what it does under the hood? And when I want to add such feature in my app, how can I get to know I should use camomile instead of other tools.

@wooorm
Copy link
Member

wooorm commented Nov 20, 2023

The original project that is unmaintained was called "camo", which is a shortening of camouflage.
This package name also starts with camo, so it's a play on that related work.

In the readme you can find a section "when should I use this?" which answers your second question!

@JounQin
Copy link
Member

JounQin commented Nov 20, 2023

OK, actually I mean the discoverability of the package. Usually projects under rehypejs use rehype-x (x is exactly what it does), right?

@Murderlon
Copy link
Member Author

It's a bit of an outlier indeed, but in this case the rehype prefix would be more confusing as it implies it being a plugin. Since it's a server, which is completely different from anything else in this org, a different name makes sense. And with a different name why not a wordplay on what it was inspired by :)

@JounQin
Copy link
Member

JounQin commented Nov 20, 2023

My point is still the discoverability of the package. I'm fine with name without rehype-. 😂

But if you think keywords in package.json is already fine, that's OK to me either.

@wooorm
Copy link
Member

wooorm commented Nov 20, 2023

Yes, we cannot use rehype-, that means plugins/presets. This is something else.

Choosing a meaningful name is nice for something like node-fetch for fetch. But when there isn’t really one word, or that name is already taken, I like using a fun name, a play on words.
Like undici for fetching.
Or MDX, or remark.

I don’t worry about discoverability. The keywords and description and the partial match of camo in camomile are fine.
And a link in the related project rehype-github-image soon, should do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants