Skip to content
Open
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
<%@ page import="org.wso2.carbon.identity.application.authentication.endpoint.util.EncodedControl" %>
<%@ page import="java.nio.charset.StandardCharsets" %>
<%@ page import="java.util.*" %>
<%@ page import="java.io.FileReader" %>
<%@ page import="java.io.BufferedReader" %>
Comment on lines +23 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Use explicit character encoding when reading the properties file.

FileReader uses the platform default encoding, which can vary across systems and lead to incorrect parsing of non-ASCII characters in language names or values.

📝 Proposed fix using explicit UTF-8 encoding

Replace the FileReader import and usage with InputStreamReader or use java.nio:

-<%@ page import="java.io.FileReader" %>
-<%@ page import="java.io.BufferedReader" %>
+<%@ page import="java.io.InputStreamReader" %>
+<%@ page import="java.io.BufferedReader" %>
+<%@ page import="java.io.FileInputStream" %>
+<%@ page import="java.nio.charset.StandardCharsets" %>

Then update line 129 to:

-        try (BufferedReader bufferedReader = new BufferedReader(new FileReader(filePath))) {
+        try (BufferedReader bufferedReader = new BufferedReader(
+                new InputStreamReader(new FileInputStream(filePath), StandardCharsets.UTF_8))) {

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
@identity-apps-core/apps/authentication-portal/src/main/webapp/includes/localize.jsp
around lines 23 - 24, The code imports and uses FileReader (which relies on
platform default encoding) causing potential mis-parsing of non-ASCII
properties; replace the FileReader usage and import in localize.jsp with an
explicit UTF-8 reader (e.g., use InputStreamReader(new FileInputStream(...),
StandardCharsets.UTF_8) or java.nio.file.Files.newBufferedReader(path,
StandardCharsets.UTF_8)) and remove the FileReader import; update the code at
the location currently referenced (around line 129) to open the properties file
with the chosen UTF-8-aware reader and keep BufferedReader for line reading.

<%@ page import="org.json.JSONObject" %>
<%@ page import="java.util.Calendar" %>
<%@ page import="org.apache.commons.lang.StringUtils" %>
Expand Down Expand Up @@ -99,6 +101,8 @@
Locale userLocale = browserLocale;
String localeFromCookie = null;
String BUNDLE = "org.wso2.carbon.identity.application.authentication.endpoint.i18n.Resources";
String SUPPORTED_LANGUAGES_ATTR = "supportedLanguages";
String LANGUAGE_SUPPORTED_COUNTRIES_ATTR = "languageSupportedCountries";
// List of screen names for retrieving text branding customizations.
List<String> screenNames = new ArrayList<>();
Expand All @@ -107,23 +111,58 @@
// Map to store default supported language codes.
// TODO: Use this map to generate the `language-switcher.jsp`.
Map<String, String> supportedLanguages = new HashMap<>();
supportedLanguages.put("en", "US");
supportedLanguages.put("fr", "FR");
supportedLanguages.put("es", "ES");
supportedLanguages.put("pt", "PT");
supportedLanguages.put("de", "DE");
supportedLanguages.put("zh", "CN");
supportedLanguages.put("ja", "JP");
List<String> languageSupportedCountries = new ArrayList<>();
languageSupportedCountries.add("US");
languageSupportedCountries.add("FR");
languageSupportedCountries.add("ES");
languageSupportedCountries.add("PT");
languageSupportedCountries.add("DE");
languageSupportedCountries.add("CN");
languageSupportedCountries.add("JP");
languageSupportedCountries.add("BR");
// Dynamically load supported languages from languageOptions.properties
// Use application scope to cache the parsed language options
Map<String, String> cachedSupportedLanguages = (Map<String, String>) request.getAttribute(SUPPORTED_LANGUAGES_ATTR);
List<String> cachedLanguageSupportedCountries = (List<String>) request.getAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR);
if (cachedSupportedLanguages != null && cachedLanguageSupportedCountries != null) {
supportedLanguages = cachedSupportedLanguages;
languageSupportedCountries = cachedLanguageSupportedCountries;
} else {
Comment on lines +116 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Critical mismatch: Comment indicates application scope but implementation uses request scope.

The comment on line 117 states "Use application scope to cache" but the implementation uses request.getAttribute/setAttribute, which is request-scoped. This means the properties file will be read and parsed on every single request, causing significant performance overhead.

Impact:

  • File I/O and parsing occur on every request
  • Unnecessary resource consumption
  • Reduced throughput and increased response time

Recommendation:
Either update the implementation to use application scope for true caching, or update the comment to accurately reflect request-scoped behavior. For production use, application-scoped caching is strongly recommended.

♻️ Proposed fix using application scope with thread-safe collections
-    // Dynamically load supported languages from languageOptions.properties
-    // Use application scope to cache the parsed language options
-    Map<String, String> cachedSupportedLanguages = (Map<String, String>) request.getAttribute(SUPPORTED_LANGUAGES_ATTR);
-    List<String> cachedLanguageSupportedCountries = (List<String>) request.getAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR);
+    // Dynamically load supported languages from languageOptions.properties
+    // Use application scope to cache the parsed language options
+    Map<String, String> cachedSupportedLanguages = (Map<String, String>) application.getAttribute(SUPPORTED_LANGUAGES_ATTR);
+    List<String> cachedLanguageSupportedCountries = (List<String>) application.getAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR);

At line 113, initialize with thread-safe collections:

-    Map<String, String> supportedLanguages = new HashMap<>();
-    List<String> languageSupportedCountries = new ArrayList<>();
+    Map<String, String> supportedLanguages = new java.util.concurrent.ConcurrentHashMap<>();
+    List<String> languageSupportedCountries = new java.util.concurrent.CopyOnWriteArrayList<>();

And at lines 160-161:

-            request.setAttribute(SUPPORTED_LANGUAGES_ATTR, supportedLanguages);
-            request.setAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR, languageSupportedCountries);
+            application.setAttribute(SUPPORTED_LANGUAGES_ATTR, supportedLanguages);
+            application.setAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR, languageSupportedCountries);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
@identity-apps-core/apps/authentication-portal/src/main/webapp/includes/localize.jsp
around lines 116 - 124, The comment promises application-scoped caching but the
code uses request.getAttribute/setAttribute; change reads/writes to use the
servlet/application scope instead (use
servletContext.getAttribute(SUPPORTED_LANGUAGES_ATTR) and
servletContext.setAttribute(...) or equivalent) so the parsed map/list are
stored application-wide, and when instantiating default containers for
supportedLanguages and languageSupportedCountries use thread-safe collections
(e.g., ConcurrentHashMap for supportedLanguages and a thread-safe List like
CopyOnWriteArrayList or Collections.synchronizedList) before parsing and storing
them under SUPPORTED_LANGUAGES_ATTR and LANGUAGE_SUPPORTED_COUNTRIES_ATTR so
parsing happens once and is safe for concurrent requests.

// Specify the file path
String filePath = application.getRealPath("/") + "/WEB-INF/classes/LanguageOptions.properties";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

File path construction may fail in certain deployment scenarios.

application.getRealPath("/") returns null when the WAR file is not extracted to the filesystem (e.g., running from a packed WAR). This will cause a NullPointerException during path construction.

Recommendation:
Use ServletContext.getResourceAsStream() for more portable resource access that works with both extracted and packed WARs.

♻️ Proposed fix using getResourceAsStream
-        // Specify the file path
-        String filePath = application.getRealPath("/") + "/WEB-INF/classes/LanguageOptions.properties";
-
-        // Use a BufferedReader to read the file content
-        try (BufferedReader bufferedReader = new BufferedReader(new FileReader(filePath))) {
+        // Load the properties file as a resource stream
+        try (InputStream inputStream = application.getResourceAsStream("/WEB-INF/classes/LanguageOptions.properties");
+             BufferedReader bufferedReader = new BufferedReader(
+                 new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
+            if (inputStream == null) {
+                throw new IOException("LanguageOptions.properties not found");
+            }

Note: This also requires adding the InputStream import:

<%@ page import="java.io.InputStream" %>

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In
@identity-apps-core/apps/authentication-portal/src/main/webapp/includes/localize.jsp
at line 126, The code constructs filePath using application.getRealPath("/")
which can be null for packed WARs; instead open LanguageOptions.properties via
the servlet context resource stream: replace the filePath + file access with
ServletContext.getResourceAsStream("/WEB-INF/classes/LanguageOptions.properties")
(or use the classloader getResourceAsStream for classpath lookup) and read the
properties from the returned InputStream; add the InputStream import and ensure
you null-check the stream and close it (or use try-with-resources) where the
filePath variable and application.getRealPath("/") are currently used.

// Use a BufferedReader to read the file content
try (BufferedReader bufferedReader = new BufferedReader(new FileReader(filePath))) {
String line;
while ((line = bufferedReader.readLine()) != null) {
// Ignore comments and empty lines
if (!line.trim().startsWith("#") && !line.trim().isEmpty()) {
// Split the line into key and value using '=' as the delimiter
String[] keyValue = line.split("=", 2);
if (keyValue.length < 2) {
continue; // Skip malformed lines
}
// Split the key further using '.' as the delimiter
String[] parts = keyValue[0].split("\\.");
if (parts.length == 0) {
continue;
}
String languageCode = parts[parts.length - 1];
// Split the value further using ',' as the delimiter
String[] values = keyValue[1].split(",");
if (values.length < 2) {
continue; // Skip lines without proper country,displayName format
}
String country = values[0];
// displayName is available in values[1] if needed in the future
// Add the values to the list.
supportedLanguages.put(languageCode, country);
if (!languageSupportedCountries.contains(country)) {
languageSupportedCountries.add(country);
}
}
}
request.setAttribute(SUPPORTED_LANGUAGES_ATTR, supportedLanguages);
request.setAttribute(LANGUAGE_SUPPORTED_COUNTRIES_ATTR, languageSupportedCountries);
} catch (Exception e) {
throw e;
}
Comment on lines +162 to +164
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Improve exception handling with logging and consider fallback behavior.

The current exception handling simply rethrows the exception without logging or providing a fallback. This creates several issues:

  1. If the properties file is missing or corrupted, the locale selection logic will fail with empty collections
  2. No logging makes debugging difficult in production
  3. If an exception occurs during parsing, partially populated collections may be stored in attributes (lines 160-161), leading to inconsistent state

Recommendation:
Add logging and consider providing fallback to ensure graceful degradation if the properties file cannot be loaded.

♻️ Proposed fix with logging and empty collection handling
         } catch (Exception e) {
-            throw e;
+            // Log the error for debugging
+            System.err.println("Failed to load LanguageOptions.properties: " + e.getMessage());
+            e.printStackTrace();
+            
+            // Ensure collections are initialized even on failure
+            // Application will fall back to default "en_US" locale
+            if (supportedLanguages.isEmpty()) {
+                supportedLanguages.put("en", "US");
+                languageSupportedCountries.add("US");
+            }
         }
     }

Alternatively, if failure to load the properties file should prevent application startup:

         } catch (Exception e) {
+            System.err.println("Critical: Failed to load LanguageOptions.properties: " + e.getMessage());
+            e.printStackTrace();
             throw e;
         }
📝 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
} catch (Exception e) {
throw e;
}
} catch (Exception e) {
// Log the error for debugging
System.err.println("Failed to load LanguageOptions.properties: " + e.getMessage());
e.printStackTrace();
// Ensure collections are initialized even on failure
// Application will fall back to default "en_US" locale
if (supportedLanguages.isEmpty()) {
supportedLanguages.put("en", "US");
languageSupportedCountries.add("US");
}
}
🤖 Prompt for AI Agents
In
@identity-apps-core/apps/authentication-portal/src/main/webapp/includes/localize.jsp
around lines 162 - 164, The catch(Exception e) block currently just rethrows the
exception; update it to log the exception (e.g., using a logger or
servletContext.log) and ensure any attributes populated earlier (the locale
collections placed into request/session) are either cleared or replaced with
safe fallbacks (empty lists or a default locale list) to avoid
partially-populated state; do this by catching the exception, calling
logger.error("Failed to load locale properties", e) (or servletContext.log) and
then set the same attribute names used earlier to empty collections or a known
default before rethrowing or handling the error gracefully.

}
// Check cookie for the user selected language first
Cookie[] cookies = request.getCookies();
Expand Down