You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@fineract.apache.org by "mcc0nnell (via GitHub)" <gi...@apache.org> on 2026/09/06 01:00:40 UTC
[PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
mcc0nnell opened a new pull request, #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133
Description
StepUpTokenService currently exposes multiple actionFingerprint overloads that serialize action parameters through separate code paths.
The current transfer initiate/confirm call sites are aligned, but keeping several overloads makes it possible for future callers to represent the same logical step-up action differently and therefore produce different fingerprints.
This change:
* replaces the duplicate overloads with a single canonical fingerprint path
* keeps BigDecimal normalization centralized using stripTrailingZeros().toPlainString()
* preserves the fingerprint values produced by the existing call shapes
* adds regression coverage for account IDs, account type, amounts, and string-only action parameters
This is intentionally behavior-preserving for current callers. The goal is to remove an API footgun and make step-up fingerprint generation deterministic through one implementation path.
Addresses #113.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. To abuse, they would need the JWT, fingerprint, and OTP which only the real user would have. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "mcc0nnell (via GitHub)" <gi...@apache.org>.
mcc0nnell commented on PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#issuecomment-5575843416
Pushed follow-ups for the Copilot review:
1. Null endpoint uses `String.valueOf(endpoint)` (behavior-preserving `"null|..."`; no NPE).
2. Fingerprint parts are fail-closed — only null / String / BigDecimal / boxed integrals; unsupported types throw `IllegalArgumentException`.
3. Regression tests cover null-endpoint equivalence and unsupported-type rejection.
Ready for another look / maintainer review when convenient.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3942561905
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,22 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ StringBuilder canonical = new StringBuilder(endpoint).append('|');
+ for (int i = 0; i < parts.length; i++) {
Review Comment:
`new StringBuilder(endpoint)` will throw a NullPointerException if `endpoint` is ever null. Previously, the canonical string was built via concatenation and would have produced the literal "null" instead, so this is a behavior change that contradicts the "behavior-preserving" goal.
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,22 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ StringBuilder canonical = new StringBuilder(endpoint).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
+ return hash(canonical.toString());
}
- public String actionFingerprint(String endpoint, String... parts) {
- String canonical = endpoint + "|" + String.join("|", parts);
- return hash(canonical);
+ private String fingerprintPart(Object part) {
+ if (part instanceof BigDecimal decimal) {
+ return decimal.stripTrailingZeros().toPlainString();
+ }
+ return String.valueOf(part);
}
Review Comment:
The new `Object...` signature makes it easy for callers to pass arbitrary objects whose `toString()` is not stable across JVM instances (e.g., default `Object#toString` includes an identity hash). In a scaled deployment, initiate/confirm may hit different instances, so fingerprints could become non-deterministic unless unsupported types are rejected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. And client isn't allowed to send free strings here anyways. Even if they did, they would need the JWT, fingerprint, and OTP to abuse. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. To abuse, they would need the JWT, fingerprint, and OTP to abuse. Feel free to ignore.
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. To abuse, they would need the JWT, fingerprint, and OTP. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#issuecomment-5580807743
hi @mcc0nnell, please refer to @Aman-Mittal 's comment on signing your PR. Otherwise, thank you for the hard work!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. Even if abused, they would need the JWT, fingerprint, and OTP which only the real user would have. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal commented on PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#issuecomment-5557695566
@mcc0nnell You should have verified commit (Signed commit for PR getting merfed)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was pre-existing. And client isn't allowed to send free strings here anyways. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "mcc0nnell (via GitHub)" <gi...@apache.org>.
mcc0nnell commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3952702769
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,22 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ StringBuilder canonical = new StringBuilder(endpoint).append('|');
+ for (int i = 0; i < parts.length; i++) {
Review Comment:
Addressed: `actionFingerprint` now builds from `String.valueOf(endpoint)`, so a null endpoint keeps the prior `"null|..."` canonical form instead of NPEing.
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,22 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ StringBuilder canonical = new StringBuilder(endpoint).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
+ return hash(canonical.toString());
}
- public String actionFingerprint(String endpoint, String... parts) {
- String canonical = endpoint + "|" + String.join("|", parts);
- return hash(canonical);
+ private String fingerprintPart(Object part) {
+ if (part instanceof BigDecimal decimal) {
+ return decimal.stripTrailingZeros().toPlainString();
+ }
+ return String.valueOf(part);
}
Review Comment:
Addressed: fingerprint parts are now fail-closed. Only `null`, `String`, `BigDecimal`, and boxed integral numbers are accepted; other types raise `IllegalArgumentException` so initiate/confirm cannot diverge on identity-based `toString()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955190665
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
I think this was a pre-existing issue. And client isn't allowed to send free strings here anyways. Feel free to ignore.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "edk12564 (via GitHub)" <gi...@apache.org>.
edk12564 commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3955206202
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
+ return hash(canonical.toString());
}
Review Comment:
I think this is unnecessary. We don't want to create fingerprints from null arguments. Better to fail closed here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133#discussion_r3952712825
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
+ return hash(canonical.toString());
}
Review Comment:
`actionFingerprint(String, Object...)` will throw a NullPointerException if a caller passes a null varargs array explicitly (e.g. `actionFingerprint(endpoint, (Object[]) null)`). Varargs methods commonly receive null arrays from accidental casts; treating null as "no parts" keeps behavior robust and avoids unexpected 500s.
##########
consumer/src/main/java/org/apache/fineract/consumer/infrastructure/stepup/service/StepUpTokenService.java:
##########
@@ -46,27 +46,34 @@ public class StepUpTokenService {
private final JwtIssuer jwtIssuer;
private final JwtDecoder jwtDecoder;
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId, BigDecimal amount) {
- String canonical =
- endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(
- String endpoint, Long fromAccountId, Long toAccountId, String toAccountType, BigDecimal amount) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId + "|" + toAccountType + "|"
- + amount.stripTrailingZeros().toPlainString();
- return hash(canonical);
- }
-
- public String actionFingerprint(String endpoint, Long fromAccountId, Long toAccountId) {
- String canonical = endpoint + "|" + fromAccountId + "|" + toAccountId;
- return hash(canonical);
+ public String actionFingerprint(String endpoint, Object... parts) {
+ // String.valueOf preserves the prior null-endpoint behavior ("null|...") without NPE.
+ StringBuilder canonical = new StringBuilder(String.valueOf(endpoint)).append('|');
+ for (int i = 0; i < parts.length; i++) {
+ if (i > 0) {
+ canonical.append('|');
+ }
+ canonical.append(fingerprintPart(parts[i]));
+ }
Review Comment:
The canonicalization uses `|` as an unescaped separator while allowing arbitrary `String` parts. If any user-controlled field contains `|`, two different logical tuples can canonicalize to the same string (e.g. parts `["a|b","c"]` vs `["a","b|c"]`), enabling an action-fingerprint collision for step-up verification. Consider an unambiguous encoding (length-prefix each part, or escape `|` and the escape char) so different part boundaries can never produce the same canonical string.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix/step up action fingerprint overloads [fineract-consumer-facing]
Posted by "Aman-Mittal (via GitHub)" <gi...@apache.org>.
Aman-Mittal merged PR #133:
URL: https://github.com/apache/fineract-consumer-facing/pull/133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org