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

Move scripttagextractor to AI #538

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Move scripttagextractor to AI #538

wants to merge 1 commit into from

Conversation

thypon
Copy link
Member

@thypon thypon commented Mar 8, 2024

I moved the script tag extractor to AI. AI production environments require pickle to be fast enough.

All the tests should pass, as they were passing before.

I feel the tradeoff is ok. /s

for f in files:
main(f, args.suffix, args.add_suffix_to_original, args.dry_run)
with open(localdir + "/scripttagextractor.pkl", 'rb') as f:
data = pickle.load(f)
Copy link

Choose a reason for hiding this comment

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

reported by reviewdog 🐶
[semgrep] Avoid using pickle, which is known to lead to code execution vulnerabilities. When unpickling, the serialized data could be manipulated to run arbitrary code. Instead, consider serializing the relevant data as JSON or a similar text-based serialization format.

Source: https://semgrep.dev/r/python.lang.security.deserialization.pickle.avoid-pickle


Cc @thypon @bcaller


for f in files:
main(f, args.suffix, args.add_suffix_to_original, args.dry_run)
with open(localdir + "/scripttagextractor.pkl", 'rb') as f:
Copy link
Member

Choose a reason for hiding this comment

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

how did you create this file?

@bcaller
Copy link
Contributor

bcaller commented May 13, 2024

I don't get it. What on Earth is this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants