Code cleanups and security improvements by moshekaplan · Pull Request #37 · scummvm/scummvm-sites

moshekaplan · 2025-11-10T04:19:15Z

No description provided.

Add dependabot

Bumps [actions/setup-python](https://github.com/actions/setup-python) from 5 to 6. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v5...v6) --- updated-dependencies: - dependency-name: actions/setup-python dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v4...v5) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [cryptography](https://github.com/pyca/cryptography) from 45.0.6 to 46.0.3. - [Changelog](https://github.com/pyca/cryptography/blob/main/CHANGELOG.rst) - [Commits](pyca/cryptography@45.0.6...46.0.3) --- updated-dependencies: - dependency-name: cryptography dependency-version: 46.0.3 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [pytest](https://github.com/pytest-dev/pytest) from 8.4.1 to 9.0.0. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@8.4.1...9.0.0) --- updated-dependencies: - dependency-name: pytest dependency-version: 9.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

Bumps [cffi](https://github.com/python-cffi/cffi) from 1.17.1 to 2.0.0. - [Release notes](https://github.com/python-cffi/cffi/releases) - [Commits](python-cffi/cffi@v1.17.1...v2.0.0) --- updated-dependencies: - dependency-name: cffi dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>

Add pytest

Bump cffi from 1.17.1 to 2.0.0

…github/workflows/actions/setup-python-6 Bump actions/setup-python from 5 to 6 in /.github/workflows

…github/workflows/actions/checkout-5 Bump actions/checkout from 4 to 5 in /.github/workflows

Bump cryptography from 45.0.6 to 46.0.3

Bump pytest from 8.4.1 to 9.0.0

Add typing job (with failure)

Clean arg parsing

args.limit_timestamps

Remove engine_last var

demonstrate enumerate

Improve logging

Update typing.yml

Move file validation to separate function

tunnelsociety

Hi, I am a beginner who is interested in this work. I have a lot of comments and questions but I am not a ScummVM team member, just some person; I'll defer to the team before commenting on certain aspects of this PR.

If it's ok, I am asking a few questions to learn about the code and changes.

(And I just wanted to indicate minor typo in a73f457 commit msg, but not sure of the most polite way to do that 😄)

tunnelsociety · 2025-11-10T18:28:46Z

.github/workflows/typing.yml

+jobs:
+  type-check:
+    runs-on: ubuntu-latest
+    continue-on-error: true  # Allow graceful failure


the commit message says "with failure" but this line seems to specifically not fail in case of error, which I guess is the opposite; do I misunderstand the commit message?

tunnelsociety · 2025-11-10T18:30:23Z

src/scripts/db_functions.py

+            "INSERT INTO game (name, engine, gameid, extra, platform, language) VALUES (%s, %s, %s, %s, %s, %s)",
+            (title, engine_last, gameid, extra, platform, lang),
        )
+        # Try to get rid of @game_last and pass the variable explicitly instead


missing TODO or FIXME or similar? or do I misunderstand this change, or this comment in particular?

tunnelsociety · 2025-11-10T18:30:58Z

src/scripts/db_functions.py

+            "INSERT INTO game (name, engine, gameid, extra, platform, language) VALUES (%s, %s, %s, %s, %s, %s)",
+            (title, engine_last, gameid, extra, platform, lang),
        )
+        # Try to get rid of @game_last and pass the variable explicitly instead


this does not seem related to commit message "Demonstrate enumerate"; should it be a separate commit?

tunnelsociety · 2025-11-10T18:32:31Z

src/scripts/db_functions.py

 def normalised_path(name):
-    """
-    Converts \ to / in filepaths, to avoid filesystem independent filepath parsing.
+    r"""


this does not seem related to commit message "Improve logging"; should it be a separate commit?

tunnelsociety · 2025-11-10T18:36:40Z

src/scripts/compute_hash.py

+logging.basicConfig(
+    level=logging.INFO,
+    format='[%(levelname)s] %(asctime)s - %(name)s - %(message)s'
+)


all commits should be "atomic" in the sense that they are parsable, buildable, etc.: so changes like 0366f2c "Add missing )" must be squashed (preferably in advance of a PR)

(more about this in ScummVM's Commit Guidelines, discussion about which I'd defer to team members)

The commits here were a bit messy. The simplest approach would be to squash this entire thing. Not sure how ScummVM team feels about that.

tunnelsociety · 2025-11-10T18:41:30Z

src/app/pagination.py

-                html += f"<td>{counter}.</td>\n"
+                html_code += "<tr>\n"
+                html_code += f"<td>{counter}.</td>\n"
                # html += f"<td>{fileset_id}</td>\n"


Would it be wise to remove or adjust [old] commented code like # html += f"<td>{fileset_id}</td>\n" as well?
(edit: i wrote "unsafe" but that might not be the case here; thus this comment has very little meaning)

I see no reason why it couldn't be removd.

tunnelsociety · 2025-11-10T18:43:18Z

src/app/fileset.py

 from src.scripts.db_functions import (
    insert_game,
    get_all_related_filesets,
-    convert_log_text_to_links,


It's a fine change, but moving the convert_log_text_to_links function is not described by commit message "Try to remove XSS from fileset"; should this be a separate commit?

Probably. I had moved it as part of doing the XSS sanitization.

tunnelsociety · 2025-11-10T18:58:36Z

It concerns me a little that we need to worry about escaping many strings in many places to inject into HTML, with the potential to forget doing it anywhere and thus to introduce a security issue. In the interest of making it easier to write safe code, what if we don't write raw HTML, but instead manipulate HTML via some library, something like td = somehtmllib.createElement('td'); td.setText("any string which will be escaped appropriately"), assuming such lib exists? (maybe there are good, simple reasons for writing our own strings of HTML.)

(Edit: I haven't analyzed attack surface. Maybe it's all a non-issue because sensitive code is only on privileged code paths where we trust what's being injected)

moshekaplan · 2025-11-10T19:02:15Z

It concerns me a little that we need to worry about escaping many strings in many places to inject into HTML, with the potential to forget doing it anywhere and thus to introduce a security issue. In the interest of making it easier to write safe code, what if we don't write raw HTML, but instead manipulate HTML via some library, something like td = somehtmllib.createElement('td'); td.setText("any string which will be escaped appropriately"), assuming such lib exists? (maybe there are good, simple reasons for writing our own strings of HTML.)

Yes, an accurate observation. Really, I'd think the ideal way is that each web endpoint should have three function calls in it:

Validate parameters
Use the parameters to obtain the per-page data
Pass the data to a Jinja template (or the like) to pass in the dynamic values

Unfortunately, this code currently generates the raw HTML (yay for server side rendering!) and so we need to do the sanitization ourselves. I simply do not have the time to restructure everything.

moshekaplan and others added 30 commits November 9, 2025 08:32

Add ruff

c9679de

Add dependabot config

3cd3ac9

Add dependabot config

b635216

Add dependabot config

fe31067

Merge pull request #1 from moshekaplan/add_dependabot

2d3ac6c

Add dependabot

Add pytest

a66e7cf

Add pytest

abe01c4

Merge pull request #7 from moshekaplan/add_pytest

c34fb36

Add pytest

Merge pull request #6 from moshekaplan/dependabot/uv/cffi-2.0.0

e5313ad

Bump cffi from 1.17.1 to 2.0.0

Merge pull request #2 from moshekaplan/dependabot/github_actions/dot-…

ab0636b

…github/workflows/actions/setup-python-6 Bump actions/setup-python from 5 to 6 in /.github/workflows

Merge pull request #3 from moshekaplan/dependabot/github_actions/dot-…

6ea3611

…github/workflows/actions/checkout-5 Bump actions/checkout from 4 to 5 in /.github/workflows

Merge pull request #4 from moshekaplan/dependabot/uv/cryptography-46.0.3

5bf83ea

Bump cryptography from 45.0.6 to 46.0.3

Merge pull request #5 from moshekaplan/dependabot/uv/pytest-9.0.0

2c5b7ce

Bump pytest from 8.4.1 to 9.0.0

Add typing job (with failure)

805c52b

Pytest: Enable more

507aa8d

Merge pull request #8 from moshekaplan/add_typing

a8122fc

Add typing job (with failure)

Clean arg parsing

4cebdc7

Merge pull request #9 from moshekaplan/clean_args

0f902ad

Clean arg parsing

Update README.md

e14f86a

Remove engine_last var

0a998c4

args.limit_timestamps

50ec507

Merge pull request #10 from moshekaplan/clean_argparsing_further

386eb13

args.limit_timestamps

Merge pull request #11 from moshekaplan/remove_engine_last

2adaa19

Remove engine_last var

demonstrate enumerate

646a9c1

Merge pull request #12 from moshekaplan/clean_loop

4e4f7eb

demonstrate enumerate

moshekaplan and others added 12 commits November 9, 2025 15:39

Improve logging

e77deea

Add missing )

0366f2c

Merge pull request #13 from moshekaplan/improve_logging

1c14f30

Improve logging

Update typing.yml

c26c857

Move file validation to separate function

b8f495a

Update typing.yml

91d7a50

Update typing.yml

77f45f1

Update typing.yml

bee0999

Merge pull request #15 from moshekaplan/moshekaplan-patch-1

91f5e09

Update typing.yml

Merge pull request #14 from moshekaplan/move_isvalidfile

8c8c48a

Move file validation to separate function

Prevent XSS on pagination

8f11aa3

Try to remove XSS form fileset

a73f457

tunnelsociety reviewed Nov 10, 2025

View reviewed changes

Conversation

moshekaplan commented Nov 10, 2025

Uh oh!

tunnelsociety left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tunnelsociety Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tunnelsociety commented Nov 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moshekaplan commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tunnelsociety Nov 10, 2025 •

edited

Loading

tunnelsociety commented Nov 10, 2025 •

edited

Loading