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

Fix multi genres #939

Merged
merged 6 commits into from
Feb 8, 2024
Merged

Fix multi genres #939

merged 6 commits into from
Feb 8, 2024

Conversation

soncaokim
Copy link
Collaborator

Add missing implementation to support multi-genre (loading and saving)

@soncaokim soncaokim added the bug label Feb 4, 2024
@soncaokim soncaokim marked this pull request as ready for review February 7, 2024 17:46
@soncaokim
Copy link
Collaborator Author

@gaycodegal There is actually more to do with mutli-genres support - I overlooked this part as well.

This PR provides necessary missing bits for that. Otherwise genres filled with '\n' (from the tag editor screens) wont be interpreted correctly.

@soncaokim soncaokim merged commit d7f538a into master Feb 8, 2024
2 checks passed
@soncaokim soncaokim deleted the fix-multi-genres branch February 8, 2024 13:55
@gaycodegal
Copy link
Contributor

@soncaokim oh thanks for the catch. I hadnt checked unicode normalization, or perhaps it was that i didnt check full app boot cycle. One question though, is Song.toString used anywhere important? it looks like something that could be vulnerable to injection attacks if its used in anything important, such as being interpreted as code. we might wish to escape our values for all fields to prevent malicious uses if this is indeed used anywhere as code / with a database

@gaycodegal
Copy link
Contributor

like for example, users can store newlines in any field regardless of if we personally want them to or not. If genres broke it, conceivably other fields may as well; we just dont test for that

@gaycodegal
Copy link
Contributor

and also what about single quote injections - does unicode normalization safeguard against that or not

@soncaokim
Copy link
Collaborator Author

Hi, as of now, the app does nothing special to prevent SQL injection, beside some common practice such as replacement parameter or query (cf https://developer.android.com/privacy-and-security/risks/sql-injection)

@gaycodegal
Copy link
Contributor

gaycodegal commented Feb 11, 2024

@soncaokim ah ok so that is SQL then. I'll fix it when I have time. From my brief reading of the code, we are not safe against malicious files, and you could theoretically make an mp3 file that causes the app to not open

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

Successfully merging this pull request may close these issues.

None yet

2 participants