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

Local .git directory is embedded in the zip file generated by info #7125

Closed
4 tasks done
zx80 opened this issue Apr 29, 2024 · 11 comments
Closed
4 tasks done

Local .git directory is embedded in the zip file generated by info #7125

zx80 opened this issue Apr 29, 2024 · 11 comments
Assignees
Labels
change request Issue requests a new feature or improvement resolved Issue is resolved, yet unreleased if open

Comments

@zx80
Copy link

zx80 commented Apr 29, 2024

Context

When generating a bug report, more things than desired are leaked out.

Bug description

The zip file generated by the info plugin include the local .git history.

This must be filtered out, because it has potential security implications.

Related links

Reproduction

9.5.19-git-directory-in-zip.zip

Steps to reproduce

Open zip generated by info and acknowledge that it contains the .git directory.

Browser

No response

Before submitting

@squidfunk
Copy link
Owner

squidfunk commented Apr 29, 2024

Thanks for reporting. Note that this runs counter to the idea of a minimal reproduction – you should not pack up your project, but create a new one which only includes the parts that show an issue. Other than that, @kamilkrzyskow, should we add .git to the list of exclusion patterns?

@squidfunk squidfunk added the bug Issue reports a bug label Apr 29, 2024
@kamilkrzyskow
Copy link
Collaborator

Sure, I believe a workflow where people prefer to delete stuff from their project on a branch to later simply git restore if a file needs restoration is worth supporting.

I'm a bit ignorant about what security sensitive info can be stored in the .git cache, perhaps names of branches so more of a privacy matter. Private keys should not be stored there or should they 🤔

The exclusion pattern should likely also include inner subprojects as they could also have .git directories.
The patterns always are resolved from the beginning so /[^/]*\.git/ should do the trick.

@squidfunk
Copy link
Owner

I'm a bit ignorant about what security sensitive info can be stored in the .git cache, perhaps names of branches so more of a privacy matter. Private keys should not be stored there or should they 🤔

I'm also not entirely sure if we want the behavior that is asked for. We have some plugins we integrate with that demand a .git repository for a working minimal reproduction, specifically:

  • git-revision-date-localized
  • git-authors
  • git-committers

It is impossible for us to know which plugin needs the git history without creating yet another inclusion list, which is impractical. Minimal reproductions with those plugin would cease to work.

The exclusion pattern should likely also include inner subprojects as they could also have .git directories.

Now that I would've forgotten 🫠 Great thinking.


Another idea: What if we just print a warning that a git repository was included? I think this should be sufficient for the author to know that it's there and decide whether it's necessary or not.

@squidfunk squidfunk added change request Issue requests a new feature or improvement and removed bug Issue reports a bug labels Apr 29, 2024
@squidfunk
Copy link
Owner

I've relabelled this issue from "bug" to "change request". It is sensical for us to include the repository for the reasons stated, but I understand that this is not what everybody might want.

@kamilkrzyskow
Copy link
Collaborator

kamilkrzyskow commented Apr 29, 2024

Maybe this is a stretch, but let's check the names of plugins if they start with git- as this is a pattern for this kind of modules.
If there are no git plugins found exclude the directory by adding the pattern dynamically.

The idea with the warning is also good as we want to make people aware they are about to post their private repository data.

This could be even an explicit input prompt to confirm saving the zip, and the user is aware what they're doing. Best to protect the project from angry developers claiming purposeful negligence with the info plugin.

@squidfunk
Copy link
Owner

Hmm, I not really for using heuristics that are based on mere conventions. I'd say we go with the warning, as the author might also not want to include the git repository, albeit his reproduction contains one of the git plugins, e.g. when another bug is reported. Since the author is explicitly invoking the mkdocs build command and looks at the output the plugin produces, so the warning can be guaranteed to be seen.

@zx80
Copy link
Author

zx80 commented Apr 30, 2024

you should not pack up your project, but create a new one which only includes the parts that show an issue.

Sure… Please consider that devs may be lazy people and do that anyway to save time 😆

@zx80
Copy link
Author

zx80 commented Apr 30, 2024

The exclusion pattern should likely also include inner subprojects as they could also have .git directories. The patterns always are resolved from the beginning so /[^/]*\.git/ should do the trick.

Maybe consider other stuff such as .gitmodules, .github/, .svn/… Basically any dot file is dubious?

@kamilkrzyskow
Copy link
Collaborator

kamilkrzyskow commented Apr 30, 2024

Please consider that devs may be lazy people and do that anyway to save time

Nowadays, It could be considered that lazy people have no rights to their privacy. Could be seen with the example of the Accept All/Let me choose my... tracking cookies on the European internet. 😃

Basically any dot file is dubious?

That was my initial idea with the recent exclusion logic rewrite.
Initially I used a .gitignore-like file to exclude .dotfiles in the root directory.

But later when I rewrote it to a more "Pythonic" regex expression list, a discussion has begun on the validity of excluding every .dotfile. The results were that, in some cases those files could be necessary to properly debug the project, and also they are sort of proof that people didn't go the extra step to reproduce their issue from scratch, which in many cases could lead to a discovery in user error, basically removing the need to send us the file altogether.

You can read the whole PR here: #6874


Your concerns have been heard and a proper warning will decrease the chance of people sending their private/sensitive data willy-nilly.

@kamilkrzyskow
Copy link
Collaborator

Addressed in f724bb9

@squidfunk squidfunk added the resolved Issue is resolved, yet unreleased if open label May 3, 2024
@squidfunk
Copy link
Owner

Released as part of 9.5.21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change request Issue requests a new feature or improvement resolved Issue is resolved, yet unreleased if open
Projects
None yet
Development

No branches or pull requests

3 participants