fix: Trust user installed SSL certs - #54
Conversation
This PR adds a network config that will allow the app to trust user installed certs that some of the Shiori users tend to use while running on local networks.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @theryecatcher, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue #43 by enabling the application to trust user-installed SSL certificates. This is particularly useful for users running Shiori on local networks who may be using self-signed certificates. The changes involve modifying the AndroidManifest.xml file to reference a new network security configuration file, network_security_config.xml, which specifies that user-added certificate authorities should be trusted.
Highlights
- Trust User Certificates: The app will now trust user-installed SSL certificates, resolving issues for users on local networks with self-signed certificates.
- Network Security Configuration: A new
network_security_config.xmlfile is added to configure network security settings, specifically trusting user-added CAs. - AndroidManifest Modification: The
AndroidManifest.xmlfile is updated to reference thenetwork_security_config.xmlfile.
Changelog
- presentation/src/main/AndroidManifest.xml
- Replaced
android:usesCleartextTraffic="true"withandroid:networkSecurityConfig="@xml/network_security_config"on line 11 to reference the network security configuration file. - Added a newline character to the end of the file.
- Replaced
- presentation/src/main/res/xml/network_security_config.xml
- Created a new file to define the network security configuration.
- Configured the base configuration to allow cleartext traffic.
- Added trust anchors to trust both system and user-added certificate authorities.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Trivia time!
What is the purpose of a Certificate Authority (CA) in the context of SSL/TLS?
Click here for the answer
A Certificate Authority (CA) is a trusted entity that issues digital certificates. These certificates are used to verify the identity of websites and other entities, ensuring secure communication over the internet using SSL/TLS.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR introduces a network security configuration to trust user-installed SSL certificates, addressing issue #43. The changes involve modifying the AndroidManifest.xml and adding a new network_security_config.xml file. Overall, the changes seem reasonable, but let's review for potential improvements and security considerations.
Summary of Findings
- Security Implications: Trusting user-installed certificates can introduce security risks if not handled carefully. It's crucial to ensure that users are aware of the implications and that the app doesn't blindly trust all user-installed certificates without proper validation.
- Cleartext Traffic: The
cleartextTrafficPermitted="true"setting in the network security configuration should be carefully considered, as it allows the app to communicate over unencrypted HTTP. This should only be enabled if absolutely necessary and with a clear understanding of the security risks.
Merge Readiness
The pull request introduces important functionality for users on local networks. However, due to the security implications of trusting user-installed certificates and allowing cleartext traffic, I recommend a thorough review of the security aspects before merging. Ensure that the app provides sufficient warnings and guidance to users about the risks involved. I am unable to directly approve this pull request, and recommend that others review and approve this code before merging.
| <base-config cleartextTrafficPermitted="true"> | ||
| <trust-anchors> | ||
| <!-- Trust preinstalled CAs --> | ||
| <certificates src="system" /> | ||
| <!-- Additionally trust user added CAs --> | ||
| <certificates src="user" /> | ||
| </trust-anchors> | ||
| </base-config> |
There was a problem hiding this comment.
The cleartextTrafficPermitted="true" setting allows the app to communicate over unencrypted HTTP. This can expose sensitive data to eavesdropping. Is this truly necessary? If so, consider limiting its scope to specific domains or network conditions. Add a comment explaining why this is enabled and what the security implications are.
<!-- Allows the app to communicate over unencrypted HTTP. Only enable if absolutely necessary and with a clear understanding of the security risks. -->
<base-config cleartextTrafficPermitted="true">
There was a problem hiding this comment.
I am keeping this config as is. Can be updated, Just wanted the app to have the same defaults that was previously present.
| <!-- Additionally trust user added CAs --> | ||
| <certificates src="user" /> |
There was a problem hiding this comment.
Trusting user-added CAs can be risky. Ensure users are aware of the implications and that the app doesn't blindly trust all user-installed certificates without proper validation. Consider adding a warning or disclaimer within the app's settings or documentation.
<!-- Additionally trust user added CAs. Ensure users are aware of the implications and risks involved. -->
<certificates src="user" />
There was a problem hiding this comment.
This is required for allowing users to run their own internal Shiori Server and serving them over their lan with self signed certificates.
| android:icon="@mipmap/ic_launcher" | ||
| android:label="@string/app_name" | ||
| android:usesCleartextTraffic="true" | ||
| android:networkSecurityConfig="@xml/network_security_config" |
There was a problem hiding this comment.
Consider adding a comment explaining why networkSecurityConfig is being added and what security implications it has for the app.
android:networkSecurityConfig="@xml/network_security_config" <!-- Allows trusting user-installed SSL certificates for local networks. -->
|
@DesarrolloAntonio : Is it possible to merge this PR? My issue #43 is over one year old now. I don't see any problems with this PR. It makes shiori to respect installed systemwide certificates by the user. For me it's obvious that an androiduser want's all apps to take self installed certificates into account. Otherwise the user won't install this certificates. BTW: Thanks to @theryecatcher for the PR! |
|
Thanks, I’ll take a look at the PR when I get some time. Regards
… El 18 jul 2025, a las 7:11, IeP4nieF ***@***.***> escribió:
IeP4nieF
left a comment
(DesarrolloAntonio/Shiori-Android-Client#54)
<#54 (comment)>
@DesarrolloAntonio <https://github.com/DesarrolloAntonio> : Is it possible to merge this PR? My issue #43 <#43> is over one year old now. I don't see any problems with this PR. It makes shiori to respect installed systemwide certificates by the user. For me it's obvious that an androiduser want's all apps to take self installed certificates into account. Otherwise the user won't install this certificates.
BTW: Thanks to @theryecatcher <https://github.com/theryecatcher> for the PR!
—
Reply to this email directly, view it on GitHub <#54 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABVNQVTK3OEW6G2ZBL3VUID3JB6ZHAVCNFSM6AAAAABZSLXHP2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTAOBWG43TENBQGQ>.
You are receiving this because you were mentioned.
|
3 participants
First of all thanks for all your work on this app. This PR adds a network config that will allow the app to trust user installed certs that some of the Shiori users tend to use while running on local networks.
This should resolve the issue #43.