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

Android Java: Add some final #13700

Closed
wants to merge 7 commits into from
Closed

Android Java: Add some final #13700

wants to merge 7 commits into from

Conversation

oong819
Copy link
Contributor

@oong819 oong819 commented Jul 29, 2023

Add compact, short information about your PR for easier understanding:

  • Goal of the PR
    add final to harden the code (its not affect mods and games, but make Minetest harder to be malicious)
  • How does the PR work?
    add final, not even tested sorry
  • Does it resolve any reported issue?
    not yet checked
  • Does this relate to a goal in the roadmap?
    probally not
  • If not a bug fix, why is this PR needed? What usecases does it solve?
    (cybersecurity-ly) harden the code, slightly improve performance

To do

This PR is Ready for Review.

  • Test it

How to test

  1. Evertthing should work fine

@oong819 oong819 marked this pull request as draft July 29, 2023 08:59
@oong819
Copy link
Contributor Author

oong819 commented Jul 29, 2023

there is c++ stuff

edit: there is so much warnings about overriden method not marked idk c++

@Zughy Zughy added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Jul 29, 2023
@oong819
Copy link
Contributor Author

oong819 commented Jul 29, 2023

XD i just added final to constructor

@oong819 oong819 marked this pull request as ready for review July 29, 2023 12:30
@srifqi
Copy link
Member

srifqi commented Aug 2, 2023

I myself am not sure about this approach (marking a lot of things as final). Other developers might have ideas about this. Maybe we can apply this like we apply const in C++ code?

The code does compile and work, though.

@oong819
Copy link
Contributor Author

oong819 commented Aug 3, 2023

if its did not got overriden or reasigned, then i think its should have final

idk if the final will protect it from memory editing or not

and i think c++ code should have that too? idk c/c++

@rollerozxa
Copy link
Member

Android APKs need to be signed before they can be installed, which both confirms its integrity and prevents tampering without having to resign. If you don't have the original key then you will need to sign with another one, which the Android system will reject if you already have the app installed with the original key.

I don't see how slapping final onto all the Java code improves security. Just like how making sensitive class variables private doesn't improve security, it's just supposed to signify that you shouldn't be extending or otherwise inheriting the class (another OOP code encapsulation technique I guess). In addition apps cannot hook into other apps' Java code like it's a library, they're all mostly self-contained and cannot access others' Java classes, so the only code that could inherit the classes would be our own.

@oong819
Copy link
Contributor Author

oong819 commented Aug 3, 2023

usually i just re-sign it with testkey

the final and private can make hacking the app much harder and time consump but Minetest are FOSS anyway so i really dont think of any use cases for this...

even for file malformed when transfering are unlikely cuz apk are compressed + signed + checksum

i'll wait for the core dev decide...

@oong819
Copy link
Contributor Author

oong819 commented Aug 3, 2023

UPDATE: based on this webpage: https://www.baeldung.com/java-final-performance final seems to improve performance

@oong819
Copy link
Contributor Author

oong819 commented Aug 3, 2023

low priority ._.

@rubenwardy
Copy link
Member

rubenwardy commented Aug 3, 2023

I'd say this would be worth doing if it fixes linter warnings. final doesn't impact security at all, it's just a code style thing

@tigercoding56
Copy link

tigercoding56 commented Aug 13, 2023

I'd say this would be worth doing if it fixes linter warnings. final doesn't impact security at all, it's just a code style thing

priorities ;P , anyway it could be a good idea to have a a easier to read description of what this PR does
(add final to harden the code -- does not make much sense for anyone who is unfamiliar with android/javascript )

, maybe "add the final keyword to android specific javascript functions in order to try and harden the code against malicious activity "

@oong819
Copy link
Contributor Author

oong819 commented Aug 16, 2023

I'd say this would be worth doing if it fixes linter warnings. final doesn't impact security at all, it's just a code style thing

priorities ;P , anyway it could be a good idea to have a a easier to read description of what this PR does (add final to harden the code -- does not make much sense for anyone who is unfamiliar with android/javascript )

, maybe "add the final keyword to android specific javascript functions in order to try and harden the code against malicious activity "

it's Java, not JavaScript. JavaScript don't have final but have const

@oong819 oong819 closed this by deleting the head repository Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Code quality Low priority Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants