Skip to content

build: enable http #162

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.

Sign up for GitHub

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

Merged
jo-elimu merged 1 commit into from
Mar 3, 2025
Merged

build: enable http #162

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/src/main/AndroidManifest.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
<permission android:name="${applicationId}.provider.READ" />

<application
android:networkSecurityConfig="@xml/network_security_config"
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Verify the network security configuration file exists and is properly configured.

Adding a network security configuration is necessary to allow HTTP cleartext traffic, which modern Android versions block by default. However, we should ensure that:

  1. The referenced XML file (@xml/network_security_config) exists in the project
  2. The configuration is properly set up to only allow cleartext traffic for specific domains
  3. This configuration is also temporary and will be removed once SSL is implemented

🏁 Script executed:

#!/bin/bash
# Check if the network_security_config.xml file exists
if [ -f "app/src/main/res/xml/network_security_config.xml" ]; then
  echo "✅ network_security_config.xml file exists."
  # Display the content of the file
  echo "Content of network_security_config.xml:"
  cat app/src/main/res/xml/network_security_config.xml
else
  echo "❌ network_security_config.xml file does not exist!"
fi

Length of output: 553

Action Required: Update Network Security Configuration to Restrict Cleartext Traffic

  • The network_security_config.xml file was found at app/src/main/res/xml/network_security_config.xml.
  • The current configuration in the file uses a <base-config> with cleartextTrafficPermitted="true", which enables cleartext traffic globally rather than for specific domains.
  • Recommendation: Update the configuration to restrict HTTP cleartext traffic to only those domains that require it. Also, document that this setup is temporary and will be removed once SSL is implemented.

android:name=".BaseApplication"
android:allowBackup="true"
android:icon="@mipmap/ic_launcher"
Expand Down
4 changes: 2 additions & 2 deletions app/src/main/java/ai/elimu/content_provider/BaseApplication.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,11 @@ public Retrofit getRetrofit() {
}

/**
* E.g. "https://hin.test.elimu.ai" or "https://hin.elimu.ai"
* E.g. "https://eng.elimu.ai" or "https://hin.elimu.ai"
*/
public String getBaseUrl() {
Language language = SharedPreferencesHelper.getLanguage(getApplicationContext());
String url = "https://" + language.getIsoCode();
String url = "http://" + language.getIsoCode();
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 3, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

❓ Verification inconclusive

Downgrading from HTTPS to HTTP introduces security risks.

Changing from HTTPS to HTTP removes transport layer security, exposing data to potential eavesdropping and man-in-the-middle attacks. While this is acceptable as a temporary measure (per PR description "temporarily enable the use of http:// for the backend until SSL is implemented"), it's recommended to add a TODO comment to ensure this gets reverted once SSL is implemented.

-        String url = "http://" + language.getIsoCode();
+        // TODO: Revert back to HTTPS once SSL is implemented in the backend (see issue #1694)
+        String url = "http://" + language.getIsoCode();

Action: Add a TODO for HTTPS reversion when SSL is implemented

The temporary downgrade to HTTP is acceptable per the PR description but still introduces security risks by removing transport layer protection. To mitigate this risk, please add a TODO comment indicating that this change should be reverted once SSL is implemented.

  • File: app/src/main/java/ai/elimu/content_provider/BaseApplication.java (Line 35)
  • Required Change:
    • Add a TODO comment above the URL assignment to remind reverting to HTTPS when backend SSL is in place.

Diff snippet for clarity:

-        String url = "http://" + language.getIsoCode();
+        // TODO: Revert back to HTTPS once SSL is implemented in the backend (see issue #1694)
+        String url = "http://" + language.getIsoCode();
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
String url = "http://" + language.getIsoCode();
// TODO: Revert back to HTTPS once SSL is implemented in the backend (see issue #1694)
String url = "http://" + language.getIsoCode();

url += ".elimu.ai";
return url;
}
Expand Down
8 changes: 8 additions & 0 deletions app/src/main/res/xml/network_security_config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<network-security-config>
<base-config cleartextTrafficPermitted="true">
<trust-anchors>
<certificates src="system" />
</trust-anchors>
</base-config>
</network-security-config>