-
Notifications
You must be signed in to change notification settings - Fork 359
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
Support presigned image URLs in markdown #7745
Support presigned image URLs in markdown #7745
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something,
Anywhere presign is hardcoded to false we need to pass to configuration parameters for presign no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eladlachmi Code looks good, thank you!
I was wondering about the package you added on npm it leads to an incorrect Github repository with 12.3K starts, that's odd!
I wanted to inspect it on Github, it's probably me missing something. Could you kindly point me to the public source code?
@Isan-Rivkin This version of the package doesn't have its own GitHub repo. It's a fork of the original package, but the forked repo isn't public for whatever reason. The creator of this package didn't update the README.md or the package.json (except for the package name), so the links are to the original repo. You can view the code on the code tab of the package page. |
@eladlachmi The code in the PR - no comments, LGTM. But, this package raises a big Red Flag to me due to few reasons:
This means that even if the package good, we will lower our overall security score of lakeFS project. |
@Isan-Rivkin I've refactored the code to move from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Looks wonderful very nice improvement!
* Support presigned image URLs in markdown * Migrate from react-markdown to rehype-react + remark-rehype
Closes #6412
This isn't a bug per se since we decided not to support pre-signed URLs when this was implemented.
This PR adds this support.
Two things to note:
react-markdown
doesn't support async plugins (see here). To get around this, I've replacedreact-markdown
with@beeswax/react-markdown-async
. This is not the official package and isn't heavily used. I've reviewed the code to ensure nothing fishy is going on. I've also locked down the package version. Since npm package versions are immutable, this shouldn't be a major security risk. However, this isn't ideal and we should plan to follow this up with a move fromreact-markdown
toreact-remark
/api/v1/repositories/first1/refs/main/objects?path=images%2Fcreate-lakefs-branch.png
. This is not the intended use case. The supported URLs for replacement arelakefs://
URIs and relative paths. Using API endpoints as image URLs in the quickstart is a mistake. We should discourage this practice because it essentially embeds specific API endpoints, path parameters, and querystring parameters in MD files. This is exactly the thing that thelakefs://
URI scheme is supposed to abstract away.