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

Provide a method to allow reading advice from file #153

Merged
merged 7 commits into from
Apr 25, 2024

Conversation

hasekawa-takumi
Copy link
Collaborator

Adds an extra, optional command line option which allows to initialize FixedAdviceProvider to the binary content of the file. This will allow easier test framework building.

@thealmarty
Copy link
Collaborator

  • is there a test for this?
  • I thought there was a security concern, which is why we are only accepting input from stdin atm. We need to check more things for files to avoid security risk.

@hasekawa-takumi
Copy link
Collaborator Author

  • is there a test for this?
  • I thought there was a security concern, which is why we are only accepting input from stdin atm. We need to check more things for files to avoid security risk.

I don't see a concern about security by using file as inputs? But I do see it is possible to pipe binary file to stdin for a same effects.

@thealmarty
Copy link
Collaborator

I don't see a concern about security by using file as inputs? But I do see it is possible to pipe binary file to stdin for a same effects.

I guess that would be scriptable too. I don't really like any of this actually >.<

@lialan lialan force-pushed the takumi/fileadvice branch from 5fc93f8 to 347fb3e Compare April 21, 2024 00:46
@morganthomas
Copy link
Collaborator

I agree this is kinda hard to test, but I also don't have any major concerns about merging this. I'm not sure what Marty's security concerns are but maybe that can be clarified.

@morganthomas morganthomas merged commit 132ccf9 into main Apr 25, 2024
2 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.

3 participants