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

Check Position Request for Primary Channel #2638

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

jp-bennett
Copy link
Collaborator

Prevents leaking location data to secondary channels.

Copy link
Member

@GUVWAF GUVWAF left a comment

Choose a reason for hiding this comment

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

I now remember that there is a way to ignore requests, which seems like a better approach than overriding sendResponse with largely duplicated logic and exposing currentReply to all modules. This should be as simple as doing the if-condition within PositionModule's allocReply, set ignoreRequest to true and return NULL (see https://github.com/meshtastic/firmware/blob/master/src/modules/NodeInfoModule.cpp#L62). Can you do it in that way?

@jp-bennett
Copy link
Collaborator Author

@GUVWAF That was how I wanted to accomplish this, but allocReply doesn't have access to the incoming request packet.
One possible approach does come to mind, where we add a data field to the postitioModule class, set it in the handleRecieveProtobuf function, and access it in allocReply. I'm not 100% sure that'll work, but it is a cleaner approach. I'll give it a shot later today.

@GUVWAF
Copy link
Member

GUVWAF commented Jul 24, 2023

Ah, you're right and you mentioned that already, sorry. Your other approach sounds good and would be cleaner indeed.

@GUVWAF
Copy link
Member

GUVWAF commented Jul 24, 2023

You could set ignoreRequest to true in handleReceivedProtobuf and return NULL in allocReply if this is the case.

Prevents leaking location data to secondary channels.
@jp-bennett
Copy link
Collaborator Author

@GUVWAF Seems to work as expected. Agreed, this is a much better approach.

Copy link
Member

@GUVWAF GUVWAF left a comment

Choose a reason for hiding this comment

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

@jp-bennett Great! Looks good to me.

@thebentern thebentern merged commit ac9c81f into meshtastic:master Jul 24, 2023
54 checks passed
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.

None yet

3 participants