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

Use fs.FS when explicitly given #5312

Merged
merged 6 commits into from
Jun 22, 2024
Merged

Conversation

doug-threatmate
Copy link
Contributor

Proposed changes

This updates DiskCatalog to use fs.FS when explicitly given by a user via NewFSCatalog.

The existing code half-used fs.FS, but there are a lot of issues with the existing implementation and what needs to be done. After doing a whole lot of testing (and concerns with golang/go#44279), I believe that the best possible implementation is a 2-track one: we support legacy os operations by default, but if someone gives us an fs.FS instance, then we'll use that exactly as they intended it.

Long-term, I hope that the fs.FS semantics are fixed by Go (or at least that a meaningful package gets built that provides full backward compatibility with the os operations).

In my particular case, I have two use cases that both work with these changes:

  1. Load files from a directory on disk for local testing. This uses NewCatalog.
  2. Load files from a tarball in memory such that the files never touch the disk (Windows Defender and other tools find them and cause all kinds of trouble, and my application really has no business writing to the disk anyway). This uses NewFSCatalog with github.com/quay/claircore/pkg/tarfs to provide the fs.FS interface.

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Copy link

@stan-threatmate stan-threatmate left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@tarunKoyalwar tarunKoyalwar left a comment

Choose a reason for hiding this comment

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

lgtm !

minor update

  • removed gologger debug statements ( in cli -debug flag enables debug statements and this flag is usually used to debug template data and not nuclei engine internals )

we are thinking of adding log/slog support soon that will allow selective filtering of different type of logs via enum/custom handler

templateFs was something i was experimenting to primarily load template directory like a virtual file system and abstract various sources via https://github.com/hairyhenderson/go-fsimpl , but it never took off because of glob or some other issue, but anyway thanks for completing the support for fs.Fs

Copy link
Member

@Mzack9999 Mzack9999 left a comment

Choose a reason for hiding this comment

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

lgtm - nice one!

@ehsandeep ehsandeep merged commit e61ca0c into projectdiscovery:dev Jun 22, 2024
12 checks passed
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.

5 participants