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

feat: allow to set password-protected notes #1481

Open
wants to merge 4 commits into
base: v4
Choose a base branch
from

Conversation

dynamotn
Copy link

@dynamotn dynamotn commented Oct 6, 2024

This PR resolve #637.

Demo

https://notes.dynamotn.dev/01_Fleeting-Notes/20240505093315 with password 123451
image

How it works

  • Key Derivation: Utilizes PBKDF2 for generating secure encryption keys.
  • Unique Salt for Each Encryption: A unique salt is generated every time the encrypt method is used, enhancing security.
  • Encryption/Decryption: Implements AES-GCM for robust data encryption and decryption.
  • Encoding/Decoding: Use base64 to convert non-textual encrypted data in HTML

@dynamotn dynamotn force-pushed the v4 branch 2 times, most recently from 5a27b0a to bdc09c9 Compare October 6, 2024 18:49
@saberzero1
Copy link
Collaborator

While I think this solution is great in what it does, I wouldn't recommend relying on it to keep anything safe. This is mostly for two reasons:

  1. Requires a plain-text password value in the frontmatter. This is poses risks for man-in-the-middle attacks if pushing to GitHub over HTTPS.
  2. Cross Fork Object Reference attacks. Most users use a fork, private or not, to host Quartz.

Having said all that, I do think that your solution has value, as password-encrypted static site can only get about as secure as your solution without external tooling/authentication. I feel like the documentation should reflect that, and I feel like we shouldn't be recommending users to deploy encrypted secret company documentation or medical records in the documentation.

@aarnphm
Copy link
Collaborator

aarnphm commented Oct 8, 2024

I just want to quickly chime in. For Quartz itself, I don't see any sort of SOC-2 compliant or adjacent security folks would use for hosting private and internal documentation (though we live in a society).

This is still a good POC imo.

@dynamotn dynamotn force-pushed the v4 branch 3 times, most recently from a9e390b to 18ded6d Compare October 10, 2024 16:22
@dynamotn
Copy link
Author

@saberzero1 @aarnphm I updated documentation and put some recommendations from you

@dynamotn dynamotn force-pushed the v4 branch 3 times, most recently from 440d910 to 717f2e4 Compare October 10, 2024 16:31
Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

tbh this can also just be kept as reference for ppl who wants to add this to there vault.

Personally I don't really find a wide usage for this.

@jackyzha0
Copy link
Owner

im happy to include it, we have quite a few reqs for this feature

been swamped the last few days but can do a quick review later

Copy link
Collaborator

@aarnphm aarnphm left a comment

Choose a reason for hiding this comment

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

To keep minimal change, I suggest we do locale in separate PR as follow up.

We can add support for Vietnamese and English should be good to start with

@dynamotn dynamotn force-pushed the v4 branch 6 times, most recently from 9f53862 to c03e45e Compare October 13, 2024 09:23
@dynamotn
Copy link
Author

dynamotn commented Nov 6, 2024

How about this PR @jackyzha0? Do you want to change anything?

@leikoilja
Copy link

came here through the search.
@dynamotn, thanks for adding this feature - i'd find it super useful.
This is not to keep smth very sensitive, but just another layer of protection for a note that i dont want to be publicly available, tho would like to be able to access from my own automation. So big kuddos to you @dynamotn for building it elegantly 🔥

@jackyzha0, let's get it in please 🙌

@sanjeed5
Copy link

sanjeed5 commented Dec 1, 2024

Came here via searching for this feature. Thanks @dynamotn

My use case is: sharing some notes with friends, but I don't want it to be accessible to everyone. I understand that if someone really wants to, they can get through, but that's okay. Won't put anything that sensitive here.

Looking forward to this!

@dynamotn dynamotn restored the v4 branch December 28, 2024 08:48
@dynamotn
Copy link
Author

I'm sorry, that's my fault when I updated new translations. I recovered it

@dynamotn dynamotn reopened this Dec 28, 2024
@lachlanjs
Copy link

Hi there,
I was interested in this feature enough that I merged it into my repo. I'm starting fresh with Quartz atm. Just wanted to share a quick bit feedback.

I assumed that this was designed for a String password. So I added password: testpwd, entered testpwd in the password prompt, and this failed. Scratched my head for a minute, then I realised I should probably put password: "testpwd". This didn't work either, until I finally tried entering "testpwd" (with the quotation marks) in the password prompt. Then I reverted to password: testpwd and did the same thing. This also works.

So my suggestion is to add some advice regarding what should be put in the password front-matter field. I see the image at the top of this thread uses a numeric field... is that what was intended?

Apologies if I missed something here! I think it's a really cool feature.

@dynamotn
Copy link
Author

dynamotn commented Jan 22, 2025

Sorry @lachlanjs. That's my bad when encoding password from frontmatter. Thank you for using my PR and testing it

@rbatra2000
Copy link

Hi! I was using this PR (it is so great thanks so much!) but when I use the search on a mobile device, the password-protected pages don't hide the text anymore. Do you have any ideas how to fix this? Thanks so much again!

image

@lachlanjs
Copy link

@rbatra2000 I just noticed the same thing this morning. Doesn't happen on my desktop:)

@lachlanjs
Copy link

I don't know whether disabling this would help?

Actually, I tried this, and the issue is not the preview window, it's the search results (I think). Each search result includes a short excerpt from the file underneath the page's title. This is also seen on desktop.

Hope this is helpful:)

@lachlanjs
Copy link

Okay @rbatra2000 I think this is what's up

itemTile.innerHTML = `<h3>${title}</h3>${htmlTags}${
  enablePreview && window.innerWidth > 600 ? "" : `<p>${content}</p>`
}`

Remove the relevant part of the string (the part that includes content in the string) and the concerned preview will not appear. Note the ternary operator. It looks like if you set enablePreview as false (as I did) then you will see the content that we were concerned with. And if the window is small (mobile, as you pointed out) then you will see, the content there too.

I've just deleted the relevant lines out of laziness and wanting to test the theory quickly, but I feel like it would make sense to include a check for whether the password feature is enabled as a part of that ternary operator.

@dynamotn I don't fully understand what's going on (I'm no web dev, data science is my thing, so I don't know Javascript/HTML/CSS in the least - which is why I am so grateful for Quartz in the first place @jackyzha0) .
I do have questions though:

  • How is it possible for this content to get displayed without the user having to provide the password somewhere? i.e. how does the content variable get filled if you haven't visited that page to provide the password?
  • What is getting encrypted? This seems slightly unclear, having not looked at the code in depth, admittedly.

Anyway, I still love the idea of the feature, and I am already happy with it the way it is - for my purposes. I don't personally need Fort Knox, just a tall hedge:). Still, it would be interesting to know how close to Fort Knox you could get with the right setup and this feature. For instance, is it that hard to avoid the issues noted by @saberzero1 above? :

  1. Requires a plain-text password value in the frontmatter. This is poses risks for man-in-the-middle attacks if pushing to GitHub over HTTPS.
  2. Cross Fork Object Reference attacks. Most users use a fork, private or not, to host Quartz.

I hope this helps in some way and I am interested to see where it goes:)

@necauqua
Copy link
Contributor

Chiming it to say that just disabling the preview on the clientside is insecure - the plain text of the note is still sent in the content index (which the search uses).

You need to encrypt/remove the content that gets added to the content index on generation here

@lachlanjs
Copy link

I guess you would only want to have the content of pages without a password included in the search. For pages with a password, I guess only the heading/tags?

If the password is in the frontmatter, then is it also getting added to the index? (L127?)

@JasonLovesDoggo
Copy link

came here through the search. @dynamotn, thanks for adding this feature - i'd find it super useful. This is not to keep smth very sensitive, but just another layer of protection for a note that i dont want to be publicly available, tho would like to be able to access from my own automation. So big kuddos to you @dynamotn for building it elegantly 🔥

@jackyzha0, let's get it in please 🙌

That's my exact use case. I won't be storing passwords or sensitive data in quartz but I would love to keep notes that I don't want everyone to be able to see while still being able to share with others.

The way I would fix the preview issue is to simply filter out all pages with the password key in the frontmatter during indexing. The page would only be avail by finding it in the tree or a direct URL.

@dynamotn
Copy link
Author

dynamotn commented Feb 4, 2025

Thanks for using my PR. I recognized some link features that may be exposed content of protected notes. I will push other commits to fix it soon.

@dynamotn
Copy link
Author

dynamotn commented Feb 4, 2025

Okay @rbatra2000 I think this is what's up

itemTile.innerHTML = `<h3>${title}</h3>${htmlTags}${
  enablePreview && window.innerWidth > 600 ? "" : `<p>${content}</p>`
}`

Remove the relevant part of the string (the part that includes content in the string) and the concerned preview will not appear. Note the ternary operator. It looks like if you set enablePreview as false (as I did) then you will see the content that we were concerned with. And if the window is small (mobile, as you pointed out) then you will see, the content there too.

I've just deleted the relevant lines out of laziness and wanting to test the theory quickly, but I feel like it would make sense to include a check for whether the password feature is enabled as a part of that ternary operator.

@dynamotn I don't fully understand what's going on (I'm no web dev, data science is my thing, so I don't know Javascript/HTML/CSS in the least - which is why I am so grateful for Quartz in the first place @jackyzha0) . I do have questions though:

* How is it possible for this content to get displayed without the user having to provide the password somewhere? i.e. how does the `content` variable get filled if you haven't visited that page to provide the password?

* _What_ is getting encrypted? This seems slightly unclear, having not looked at the code in depth, admittedly.

Anyway, I still love the idea of the feature, and I am already happy with it the way it is - for my purposes. I don't personally need Fort Knox, just a tall hedge:). Still, it would be interesting to know how close to Fort Knox you could get with the right setup and this feature. For instance, is it that hard to avoid the issues noted by @saberzero1 above? :

  1. Requires a plain-text password value in the frontmatter. This is poses risks for man-in-the-middle attacks if pushing to GitHub over HTTPS.
  2. Cross Fork Object Reference attacks. Most users use a fork, private or not, to host Quartz.

I hope this helps in some way and I am interested to see where it goes:)

I'm busy resolving another matter in my life so I'll explain it in another comment. But shortly, if you don't publish your raw vault (notes) to public, and just deploy your Quartz to static sites, your protected note's content will be encrypted. Although like you, I'm not professional TypeScript developer (I'm just a DevOps/Security specialist), I'm confident about the logic of this PR.

Immediately, I will fix bug that exposed content on index feature first to resolve for @necauqua and @rbatra2000. After that, I will take a look back all features to avoid any problems in the future.

@dynamotn
Copy link
Author

dynamotn commented Feb 6, 2025

I pushed fixed commit for @rbatra2000. Please test it and feedback if has any problem.

@dynamotn dynamotn reopened this Feb 6, 2025
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.

Password-protect notes