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

Local Deployment + Location management + HEALTH connectivity + bug fixes #229

Closed
wants to merge 22 commits into from

Conversation

bardram
Copy link
Contributor

@bardram bardram commented Feb 12, 2024

Copy link

gitguardian bot commented Feb 12, 2024

⚠️ GitGuardian has uncovered 6 secrets following the scan of your pull request.

Please consider investigating the findings and remediating the incidents. Failure to do so may lead to compromising the associated services or software components.

🔎 Detected hardcoded secrets in your pull request
GitGuardian id GitGuardian status Secret Commit Filename
7056958 Triggered Generic High Entropy Secret cbccc5b lib/local/local_study_protocol_manager.dart View secret
7056958 Triggered Generic High Entropy Secret b1882d0 lib/local/local_study_protocol_manager.dart View secret
7056958 Triggered Generic High Entropy Secret b1882d0 assets/carp/resources/protocol.json View secret
9412144 Triggered Generic High Entropy Secret b1882d0 lib/local/local_study_protocol_manager.dart View secret
9412144 Triggered Generic High Entropy Secret cbccc5b lib/local/local_study_protocol_manager.dart View secret
7056957 Triggered Generic High Entropy Secret b1882d0 assets/carp/resources/protocol.json View secret
🛠 Guidelines to remediate hardcoded secrets
  1. Understand the implications of revoking this secret by investigating where it is used in your code.
  2. Replace and store your secrets safely. Learn here the best practices.
  3. Revoke and rotate these secrets.
  4. If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.

To avoid such incidents in the future consider


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

: firstRoute),
redirect: (context, state) {
if (bloc.deploymentMode != DeploymentMode.local) {
// check authentication to CAWS
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd say remove unnecessary comments, it's implicit in the logic

Comment on lines -121 to +133
redirect: (context, state) => bloc.hasInformedConsentBeenAccepted
? firstRoute
: (bloc.studyId == null ? InvitationListPage.route : null),
// redirect: (context, state) => bloc.hasInformedConsentBeenAccepted
// ? firstRoute
// : (bloc.studyId == null ? InvitationListPage.route : null),
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the user gets redirected to the ic page through deep linking for example, do they have to accept it again?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would say yes - the reason to navigate to the IC is to show it to the user (again). This is actually a missing features right now, that the user should be able to go back and see the IC - e.g. from the setting menu.

@@ -40,14 +40,17 @@ import 'package:carp_esense_package/esense.dart';
import 'package:carp_polar_package/carp_polar_package.dart';
import 'package:research_package/research_package.dart';
import 'package:cognition_package/cognition_package.dart';
import 'package:carp_health_package/health_package.dart';
// import 'package:health/health.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

remove dead code comments

@@ -46,7 +46,7 @@ dependencies:
go_router: ^13.0.0
flutter_svg: ^2.0.4
flutter_blue_plus: ^1.15.5
intl: ^0.18.1
intl: '>=0.18.0 <1.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

are you sure about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - there has been some discussion on the intl package and problem with Flutter - not solved yet, so this is the current work-around.

@LarsRefsgaard
Copy link
Contributor

xcode is not building with these commits... and also gitguardian is complaining about exposed secrets, these need to be remedied

@LarsRefsgaard
Copy link
Contributor

maybe next time it would be beneficial to keep PRs on a smaller scope so there's not 2500 code changes to look through...

@Panosfunk
Copy link
Collaborator

xcode is not building with these commits... and also gitguardian is complaining about exposed secrets, these need to be remedied

I had the same issue last week(uploaded secrets). I think its better if we changed the history of the commits too so that the keys do not appear in old commits.

I fixed this using this link. It's a tool that automates a lot of the process but I still needed to do some things on my own.

@bardram
Copy link
Contributor Author

bardram commented Feb 21, 2024

maybe next time it would be beneficial to keep PRs on a smaller scope so there's not 2500 code changes to look through...

I agree - but the problem is that often you "touch" many files just in order to make a "small" change..

@LarsRefsgaard
Copy link
Contributor

maybe next time it would be beneficial to keep PRs on a smaller scope so there's not 2500 code changes to look through...

I agree - but the problem is that often you "touch" many files just in order to make a "small" change..

Sure, but cannot accept this PR before the app is properly building on iOS.

@bardram bardram mentioned this pull request Mar 3, 2024
@bardram
Copy link
Contributor Author

bardram commented Apr 25, 2024

This was included in PR #234

@bardram bardram closed this Apr 25, 2024
@Panosfunk Panosfunk deleted the bardram/local-deployment branch November 10, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants